feat: api client and spec improvements #265
Conversation
jason-lynch
changed the title
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a CLI to inject JSON struct tags, applies g.Meta("struct:tag:json", "...") annotations across API design files, introduces DatabaseSummary and updates ListDatabases to return summaries, updates server converters, and runs the tags tool as a pre-generation Makefile step. Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/apiv1/design/database.go`:
- Around line 1340-1344: The ListDatabasesResponse currently marks the
"databases" attribute with `json:"databases,omitempty"` which will omit the key
when the slice is empty; update the ListDatabasesResponse definition so the
"databases" attribute does not use `omitempty` (i.e., remove the struct tag on
the databases attribute in the g.Type declaration for ListDatabasesResponse that
references DatabaseSummary) so clients receive {"databases": []} instead of {}
when there are no databases.
In `@server/internal/api/apiv1/convert.go`:
- Line 206: Comment is misleading: update the inline comment above the sort to
reflect the actual keys used (it currently says "Sort by node ID" but the code
sorts by NodeName then ID). Change the comment to clearly state the real sort
order (e.g., "Sort by NodeName, then ID ascending" or "Sort by node name, then
instance ID ascending") so it matches the sort implementation that uses NodeName
and ID.
🧹 Nitpick comments (3)
api/tags/main.go (3)
205-211:selectorMatchesdoesn't verify the receiver isg.This matches any
<ident>.Type(...)call, not justg.Type(...). In practice the design files are tightly scoped so this is unlikely to cause false positives, but adding a check onsel.Xwould make the tool more robust if the design files ever import additional packages.Proposed fix
func selectorMatches(call *ast.CallExpr, name string) bool { sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { return false } - return sel.Sel.Name == name + ident, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + return ident.Name == "g" && sel.Sel.Name == name }
246-255: Preferstrconv.Quoteover manual quote wrapping.The manual prefix/suffix check doesn't handle edge cases (e.g., a value that happens to start with
"but isn't properly quoted, or values containing special characters). SincetagValuecurrently only produces simple strings this is safe today, butstrconv.Quotewould be strictly correct and simpler.Proposed fix
func stringLit(value string) *ast.BasicLit { - if !strings.HasPrefix(value, `"`) { - value = `"` + value - } - if !strings.HasSuffix(value, `"`) { - value = value + `"` - } - - return &ast.BasicLit{Kind: token.STRING, Value: value} + return &ast.BasicLit{Kind: token.STRING, Value: strconv.Quote(value)} }
143-182: Attributes without a config closure are silently skipped.If an attribute is defined without a
FuncLitargument (e.g.,g.Attribute("name", g.String)), the loop at lines 155–181 finds noFuncLitto attach the meta tag to. The tool silently produces no tag for that attribute.This might be intentional (such attributes may not exist in practice), but consider logging a warning so the omission is visible during generation. Otherwise, a new field added without a closure would silently lack a JSON tag.
Reshapes the list-databases response for two main reasons: - Goa's views do not become separate types in the OpenAPI output, which causes problems for every other generated client. - To add more fields to the instances in the list to reduce the number of calls to get instance role and other information for all instances managed by the cluster. PLAT-426
Standardizes the format of `Attribute` calls in the API design so that we can more easily write tools to modify these calls.
jason-lynch
force-pushed
the
feat/api-client-improvements
branch
from
6378384 to
12885ad
Compare
Goa produces separate structs for the client, server, and programmatic users. The latter structs do not have JSON tags, which makes it difficult to use the client libraries in some use-cases, such as CLIs. In order to make it easier to consistently maintain these tags, I've added a tool that programmatically edits the API design files to add tags to every attribute.
We were erroneously including the error field in the instance reponses, even though it was empty, because it was non-nil. This fixes it so that we only set error if it's both non-nil and non-empty.
jason-lynch
force-pushed
the
feat/api-client-improvements
branch
from
12885ad to
95506b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/tags/main.go`:
- Around line 79-99: processResultType currently only processes Attributes
blocks and skips top-level g.Attribute/g.ErrorName calls; update it to mirror
processType by also calling extractCall(stmt, "Attribute", "ErrorName") for each
stmt in the func literal body and, when non-nil, invoke processAttributes(fset,
attr, required) (using the same required value from extractRequired(flit)),
returning any error; keep the existing second-pass handling of "Attributes"
blocks so both direct Attribute/ErrorName calls and Attributes(...) blocks are
processed.
🧹 Nitpick comments (2)
api/tags/main.go (1)
64-77: Consider preserving original file permissions.
os.WriteFilewith hardcoded0o644will overwrite whatever permissions the file originally had. For a codegen tool this is generally fine, but if you want to be safe:♻️ Suggested improvement
+ info, err := os.Stat(path) + if err != nil { + return fmt.Errorf("failed to stat file '%s': %w", path, err) + } + // Write to a string first in case of errors var buf strings.Builder err = format.Node(&buf, fset, f) if err != nil { return fmt.Errorf("failed to format file: %w", err) } - err = os.WriteFile(path, []byte(buf.String()), 0o644) + err = os.WriteFile(path, []byte(buf.String()), info.Mode().Perm())api/apiv1/design/database.go (1)
575-622:DatabaseSummaryduplicates most ofDatabase— consider using Goa'sExtendDSL.
DatabaseSummaryrepeats five attributes verbatim fromDatabase(id, tenant_id, created_at, updated_at, state, instances) with identical descriptions, examples, and tags. If any of these fields change (e.g., a new enum value forstate), both types must be updated in lockstep.Goa supports
g.Extend(Database)to inherit attributes, after which you could removespecfrom the required set or simply not re-declare it. This would reduce duplication and drift risk.That said, if the team prefers explicit declarations for generated-type clarity, this is fine to keep as-is.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/plugins/add_openapi_default_response.go`:
- Around line 58-64: Remove the redundant Required field from the Schema when
setting the default response with a $ref: in
api/plugins/add_openapi_default_response.go locate the assignment to
operation.Responses["default"] creating an openapiv2.Response with a Schema that
has Ref set to "#/definitions/APIError" and delete the Required:
[]string{"name", "message"} entry so the Schema only contains the Ref (keep
Description and Schema structure intact); rely on the APIError definition for
required constraints.
- Around line 51-65: addDefaultV2 and addDefaultV3 assume operation.Responses is
non-nil and will panic on write; add a defensive nil-check in each function: if
operation.Responses is nil, initialize it to an empty map before
checking/assigning entries so writes to operation.Responses["default"] succeed;
update both addDefaultV2 (using openapiv2.Response) and addDefaultV3 (using
openapiv3.Response) to perform this initialization prior to reading or writing
operation.Responses.
5abe16e to
5918fd3
Compare
Some code generator features, such as ogen's "convenient error types" require the `default` response. Including a `default` response is best practice, but Goa doesn't have a built-in way to specify the default response. This commit adds another Goa plugin that modifies the OpenAPI files before they're templated.
jason-lynch
force-pushed
the
feat/api-client-improvements
branch
from
5918fd3 to
38b217c
Compare
1 participant
Summary
This PR addresses a few issues I ran into while trying to use our OpenAPI spec and our generated client code:
service.goor the OpenAPI spec. This makes it difficult for clients to know which subset of fields will be returned. Our "abbreviated" instance view also excluded some required fields on the base type, which causes errors for any generated client that validates server responses. I fixed this by adding a dedicatedDatabaseSummarytype. I ended up just using the baseInstancetype inDatabaseSummary.Instancesbecause it addresses PLAT-426.service.gothat are used by ourclientpackage were missing JSON struct tags. This makes it difficult to write a tool that reads/writes these structs as JSON using theclientpackage, such CLIs or other automated API clients. The simplest solution was to addg.Meta("struct:tag:json", "<struct tag>")calls to every field. In order to make this easier to maintain and less error-prone, I wrote a small tool to make these edits. This tool runs as part of themake -C api generatetarget.errorfield was always non-nil, even when it was an empty string. This was just a missing condition where we were populating this field.defaultresponse on every endpoint. This is best practice, but Goa doesn't have a built-in mechanism to add it. I've added another small plugin that adds default responses to every endpoint.Changes
list-databasesresponsedefaultresponses to all endpointsTesting
To test the new instance fields:
Notes for Reviewers
The API changes are additive-only, so the
clientpackage should be backward compatible with older Control Plane instances.PLAT-426