impl(sidekick): capture query and path parameters#2282
impl(sidekick): capture query and path parameters#2282coryan wants to merge 2 commits intogoogleapis:mainfrom
Conversation
Capture the query and path parameters of each method into a synthetic request message.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2282 +/- ##
==========================================
+ Coverage 83.99% 84.06% +0.07%
==========================================
Files 96 96
Lines 9632 9679 +47
==========================================
+ Hits 8090 8137 +47
Misses 1197 1197
Partials 345 345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
FWIW, I agree with the output of the generation that you shared with me.
The way we are achieving it in the model seems hacky / too clever to me.
I don't want to block progress. I get that the specs don't line up and we are only going to do this hack once*. I think some comments explaining the strategy could help.
*: Well, the next language might get confused when they need to generate their compute library.
| if bodyID != ".google.protobuf.Empty" { | ||
| body := &api.Field{ | ||
| Documentation: fmt.Sprintf("Synthetic request body field for the [%s()][%s] method.", input.Name, id[1:]), | ||
| Name: "body", |
There was a problem hiding this comment.
comment: this could conflict with a path/query params called body. We could deal with it, if that ever arises.
optional: report an error in sidekick now? (rather than rely on cargo catching the build error in the CI)
| Codec any | ||
| } | ||
|
|
||
| // HasRequests is used by mustache templates to optional emit code depending on |
| func makeServiceMethods(model *api.API, serviceName, serviceID string, doc *document, resource *resource) ([]*api.Method, []*api.Message, error) { | ||
| requests := &api.Message{ | ||
| Name: serviceName, | ||
| ID: serviceID, |
There was a problem hiding this comment.
repeating IDs? 😒
I know why we are doing it. It just gives me the ick.
|
|
||
| func makeServiceMethods(model *api.API, serviceID string, doc *document, resource *resource) ([]*api.Method, error) { | ||
| func makeServiceMethods(model *api.API, serviceName, serviceID string, doc *document, resource *resource) ([]*api.Method, []*api.Message, error) { | ||
| requests := &api.Message{ |
There was a problem hiding this comment.
if we are going to go with this, can you add comments describing that
- this is a parent message. child message generation goes poorly if there is not a parent.
- it will not be generated; it is just for organization
- we use it so the synthetic requests for a service can be scoped to a module (i.e.
zones::ListRequestvs.ZonesListRequest)
| // Sidekick creates synthetic messages for these specifications. The codecs | ||
| // need to generate these messages separate from "normal" messages. They | ||
| // should keep in mind that name clashes are possible. | ||
| Requests []*Message |
There was a problem hiding this comment.
the alternative is to add a Synthetic: bool field to an api.Message. And then in generation, we could skip generating serde for any message where m.Synthetic == true.
And we can skip generating the message at all when m.Synthetic == true && !m.Parent. Or something.
| // do something special to avoid clashes. | ||
| index := slices.IndexFunc(model.Messages, func(a *api.Message) bool { return a.ID == id }) | ||
| if index != -1 { | ||
| t.Errorf("synthetic request messages should not be in the `model.Messages` list") |
There was a problem hiding this comment.
I feel like they should be in the list, they should just be tagged as synthetic. 🤷
2 participants
Capture the query and path parameters of each method into a synthetic
request message.
Fixes #2275 and fixes #2267