refactor(internal/sidekick/surfer): remove remaining builder boiler plate#5800
refactor(internal/sidekick/surfer): remove remaining builder boiler plate#5800sarahheacock wants to merge 4 commits into
Conversation
Finish implementation of operations commands. - Infers resource from operation method patterns to populate resource argument. - Adds new mixins file to provide missing information such as documentation. - Build wait command from get method - Skip return_partial_success field since not available on all APIs Fixes googleapis#5485
Remove object-oriented builders with stateless functions that pass context objects when needed - Original Builder structs added unnecessary boiler plate and allowed for possible state - Parameter reduction: Context structs are only passed whenever 3 or more arguments are passed allowing for easy mistakes Code is functionally the same but allows for same pattern through out
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for synthesizing 'wait' commands for long-running operations (LRO) and improves the handling of LRO resources. It refactors the builder logic to use context objects (ArgumentContext, CommandContext, GroupContext) and adds a new mixin.go provider to handle LRO-specific logic like resource inference and fallback documentation. Feedback was provided regarding the performance of GetAllResources, which performs a full scan of the API model and is currently called repeatedly during the surface building process; it is recommended to pre-compute or cache this list to avoid potential degradation on large API models.
| func GetAllResources(model *api.API) []*api.Resource { | ||
| var resources []*api.Resource | ||
| resources = append(resources, model.ResourceDefinitions...) | ||
|
|
||
| // Add message-level resources if not already present | ||
| for _, m := range model.Messages { | ||
| if m.Resource != nil { | ||
| resources = append(resources, m.Resource) | ||
| } | ||
| } | ||
|
|
||
| // Infer operations resources from GetOperation methods | ||
| for _, s := range model.Services { | ||
| for _, m := range s.Methods { | ||
| if m.Name == GetOperation && IsOperationsMethod(m) { | ||
| res, err := InferOperationResource(m) | ||
| if err != nil { | ||
| log.Printf("WARNING: failed to infer operations resource for method %q: %v", m.ID, err) | ||
| continue | ||
| } | ||
| if res != nil { | ||
| resources = append(resources, res) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return resources | ||
| } |
There was a problem hiding this comment.
The GetAllResources function performs a full scan of the API model (resource definitions, messages, and services) to aggregate all resources, including inferred operations resources. This function is called repeatedly within loops when building the command surface (e.g., for every field of every method). For large API models, this repeated computation could lead to significant performance degradation during code generation. Consider computing the full list of resources once at the beginning of the surface building process (e.g., in buildSurface) and passing this pre-computed list down through the context objects, or caching the result of GetAllResources if the model object is immutable.
| for _, method := range service.Methods { | ||
| if err := insert(root, gb, method); err != nil { | ||
| if err := insert(root, ctx, method); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The insert function, and consequently the functions it calls (like buildCommand, newArguments, categorizeFields, GetResourceForMethod), implicitly trigger repeated calls to provider.GetAllResources(ctx.Model). As noted in the comment on provider/resource.go, GetAllResources performs a full scan of the API model. Calling it for every method and potentially every field within a method can be inefficient for large API definitions. To improve performance, consider computing the complete list of resources (using provider.GetAllResources) once at the start of buildSurface and then passing this pre-computed list through the GroupContext and CommandContext objects. This would avoid redundant model scans.
1 participant
Replace object-oriented builders with stateless functions that pass context objects when needed