Skip to content

Commit 4a3664a

Browse files
coryancoryan
authored
feat(sidekick): override pagination items field (#2441)
In one API (SqlAdmin) the default field for pagination is completely wrong: https://github.com/googleapis/googleapis/blob/a4ded737527f57207f37ba9e9c9b44732a0eb70f/google/cloud/sql/v1/cloud_sql_instances.proto#L857-L861 We need a mechanism to override the default choice. Part of the work for googleapis/google-cloud-rust#3430
1 parent 475a62d commit 4a3664a

File tree

11 files changed

+199
-28
lines changed

11 files changed

+199
-28
lines changed

‎internal/sidekick/internal/config/config.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,23 @@ type DocumentationOverride struct {
5151
Replace string `toml:"replace"`
5252
}
5353

54+
// PaginationOverrides describes overrides for pagination config of a method.
55+
type PaginationOverride struct {
56+
// The method ID.
57+
ID string `toml:"id"`
58+
// The name of the field used for `items`.
59+
ItemField string `toml:"item-field"`
60+
}
61+
5462
// Config is the main configuration struct.
5563
type Config struct {
5664
General GeneralConfig `toml:"general"`
5765

58-
Source map[string]string `toml:"source,omitempty"`
59-
Codec map[string]string `toml:"codec,omitempty"`
60-
CommentOverrides []DocumentationOverride `toml:"documentation-overrides,omitempty"`
61-
Release *Release `toml:"release,omitempty"`
66+
Source map[string]string `toml:"source,omitempty"`
67+
Codec map[string]string `toml:"codec,omitempty"`
68+
CommentOverrides []DocumentationOverride `toml:"documentation-overrides,omitempty"`
69+
PaginationOverrides []PaginationOverride `toml:"pagination-overrides,omitempty"`
70+
Release *Release `toml:"release,omitempty"`
6271

6372
// Gcloud is used to pass data into gcloud.Generate. It does not use the
6473
// normal .sidekick.toml file, but instead reads a gcloud.yaml file.
@@ -131,9 +140,10 @@ func mergeConfigs(rootConfig, local *Config) *Config {
131140
SpecificationFormat: rootConfig.General.SpecificationFormat,
132141
IgnoredDirectories: rootConfig.General.IgnoredDirectories,
133142
},
134-
Source: map[string]string{},
135-
Codec: map[string]string{},
136-
CommentOverrides: local.CommentOverrides,
143+
Source: map[string]string{},
144+
Codec: map[string]string{},
145+
CommentOverrides: local.CommentOverrides,
146+
PaginationOverrides: local.PaginationOverrides,
137147
// Release does not accept local overrides
138148
Release: rootConfig.Release,
139149
}

‎internal/sidekick/internal/config/config_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,61 @@ func TestMergeLocalForDocumentationOverrides(t *testing.T) {
260260
}
261261
}
262262

263+
func TestMergeLocalForPaginationOverrides(t *testing.T) {
264+
root := Config{
265+
General: GeneralConfig{
266+
Language: "root-language",
267+
SpecificationFormat: "root-specification-format",
268+
},
269+
PaginationOverrides: []PaginationOverride{
270+
{
271+
ID: ".package.Service.FromGlobal",
272+
ItemField: "features",
273+
},
274+
},
275+
}
276+
277+
local := Config{
278+
General: GeneralConfig{
279+
Language: "local-language",
280+
SpecificationFormat: "local-specification-format",
281+
SpecificationSource: "local-specification-source",
282+
ServiceConfig: "local-service-config",
283+
},
284+
PaginationOverrides: []PaginationOverride{
285+
{
286+
ID: ".google.sql.v1.SqlAdmin.ListInstances",
287+
ItemField: "items",
288+
},
289+
},
290+
}
291+
292+
got, err := mergeTestConfigs(t, &root, &local)
293+
if err != nil {
294+
t.Fatal(err)
295+
}
296+
want := &Config{
297+
General: GeneralConfig{
298+
Language: "local-language",
299+
SpecificationFormat: "local-specification-format",
300+
SpecificationSource: "local-specification-source",
301+
ServiceConfig: "local-service-config",
302+
},
303+
Codec: map[string]string{},
304+
Source: map[string]string{},
305+
PaginationOverrides: []PaginationOverride{
306+
{
307+
ID: ".google.sql.v1.SqlAdmin.ListInstances",
308+
ItemField: "items",
309+
},
310+
},
311+
}
312+
313+
if diff := cmp.Diff(want, got); len(diff) != 0 {
314+
t.Errorf("mismatched merged config (-want, +got):\n%s", diff)
315+
}
316+
}
317+
263318
func TestMergeConfigAndFileBadRead(t *testing.T) {
264319
root := Config{
265320
General: GeneralConfig{

‎internal/sidekick/internal/parser/disco.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ func ParseDisco(source, serviceConfigFile string, options map[string]string) (*a
5555
if err != nil {
5656
return nil, err
5757
}
58-
updateMethodPagination(result)
5958
updateAutoPopulatedFields(serviceConfig, result)
6059
return result, nil
6160
}

‎internal/sidekick/internal/parser/disco_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ func TestDisco_ParsePagination(t *testing.T) {
9393
if err != nil {
9494
t.Fatal(err)
9595
}
96+
updateMethodPagination(nil, model)
9697
wantID := "..zones.list"
9798
got, ok := model.State.MethodByID[wantID]
9899
if !ok {

‎internal/sidekick/internal/parser/openapi.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,6 @@ func makeAPIForOpenAPI(serviceConfig *serviceconfig.Service, model *libopenapi.D
129129
if err != nil {
130130
return nil, err
131131
}
132-
updateMethodPagination(result)
133132
updateAutoPopulatedFields(serviceConfig, result)
134133
return result, nil
135134
}

‎internal/sidekick/internal/parser/openapi_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,7 @@ func openapiSecretManagerAPI(t *testing.T) *api.API {
537537
if err != nil {
538538
t.Fatalf("Error in makeAPI() %q", err)
539539
}
540+
updateMethodPagination(nil, test)
540541
return test
541542
}
542543

@@ -947,6 +948,7 @@ func TestOpenAPI_Pagination(t *testing.T) {
947948
if err != nil {
948949
t.Fatalf("Error in makeAPI() %q", err)
949950
}
951+
updateMethodPagination(nil, test)
950952

951953
service, ok := test.State.ServiceByID["..Service"]
952954
if !ok {

‎internal/sidekick/internal/parser/pagination.go‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@
1515
package parser
1616

1717
import (
18+
"slices"
19+
1820
"github.com/googleapis/librarian/internal/sidekick/internal/api"
21+
"github.com/googleapis/librarian/internal/sidekick/internal/config"
1922
)
2023

2124
const (
@@ -27,7 +30,7 @@ const (
2730

2831
// updateMethodPagination marks all methods that conform to
2932
// [AIP-4233](https://google.aip.dev/client-libraries/4233) as pageable.
30-
func updateMethodPagination(a *api.API) {
33+
func updateMethodPagination(overrides []config.PaginationOverride, a *api.API) {
3134
for _, m := range a.State.MethodByID {
3235
reqMsg := a.State.MessageByID[m.InputTypeID]
3336
pageTokenField := paginationRequestInfo(reqMsg)
@@ -36,7 +39,7 @@ func updateMethodPagination(a *api.API) {
3639
}
3740

3841
respMsg := a.State.MessageByID[m.OutputTypeID]
39-
paginationInfo := paginationResponseInfo(respMsg)
42+
paginationInfo := paginationResponseInfo(overrides, m.ID, respMsg)
4043
if paginationInfo == nil {
4144
continue
4245
}
@@ -93,11 +96,11 @@ func paginationRequestToken(request *api.Message) *api.Field {
9396
return nil
9497
}
9598

96-
func paginationResponseInfo(response *api.Message) *api.PaginationInfo {
99+
func paginationResponseInfo(overrides []config.PaginationOverride, methodID string, response *api.Message) *api.PaginationInfo {
97100
if response == nil {
98101
return nil
99102
}
100-
pageableItem := paginationResponseItem(response)
103+
pageableItem := paginationResponseItem(overrides, methodID, response)
101104
nextPageToken := paginationResponseNextPageToken(response)
102105
if pageableItem == nil || nextPageToken == nil {
103106
return nil
@@ -108,7 +111,17 @@ func paginationResponseInfo(response *api.Message) *api.PaginationInfo {
108111
}
109112
}
110113

111-
func paginationResponseItem(response *api.Message) *api.Field {
114+
func paginationResponseItem(overrides []config.PaginationOverride, methodID string, response *api.Message) *api.Field {
115+
idx := slices.IndexFunc(overrides, func(o config.PaginationOverride) bool { return o.ID == methodID })
116+
if idx != -1 {
117+
overrideName := overrides[idx].ItemField
118+
fieldIdx := slices.IndexFunc(response.Fields, func(f *api.Field) bool { return f.Name == overrideName })
119+
if fieldIdx == -1 {
120+
return nil
121+
}
122+
return response.Fields[fieldIdx]
123+
}
124+
112125
for _, field := range response.Fields {
113126
if field.Repeated && field.Typez == api.MESSAGE_TYPE {
114127
return field

‎internal/sidekick/internal/parser/pagination_test.go‎

Lines changed: 104 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919

2020
"github.com/google/go-cmp/cmp"
2121
"github.com/googleapis/librarian/internal/sidekick/internal/api"
22+
"github.com/googleapis/librarian/internal/sidekick/internal/config"
2223
)
2324

2425
func TestPageSimple(t *testing.T) {
@@ -82,7 +83,7 @@ func TestPageSimple(t *testing.T) {
8283
Methods: []*api.Method{method},
8384
}
8485
model := api.NewTestAPI([]*api.Message{request, response, resource}, []*api.Enum{}, []*api.Service{service})
85-
updateMethodPagination(model)
86+
updateMethodPagination(nil, model)
8687
if method.Pagination != request.Fields[1] {
8788
t.Errorf("mismatch, want=%v, got=%v", request.Fields[1], method.Pagination)
8889
}
@@ -95,6 +96,91 @@ func TestPageSimple(t *testing.T) {
9596
}
9697
}
9798

99+
func TestPageWithOverride(t *testing.T) {
100+
resource := &api.Message{
101+
Name: "Resource",
102+
ID: ".package.Resource",
103+
}
104+
request := &api.Message{
105+
Name: "Request",
106+
ID: ".package.Request",
107+
Fields: []*api.Field{
108+
{
109+
Name: "parent",
110+
JSONName: "parent",
111+
ID: ".package.Request.parent",
112+
Typez: api.STRING_TYPE,
113+
},
114+
{
115+
Name: "page_token",
116+
JSONName: "pageToken",
117+
ID: ".package.Request.pageToken",
118+
Typez: api.STRING_TYPE,
119+
},
120+
{
121+
Name: "page_size",
122+
JSONName: "pageSize",
123+
ID: ".package.Request.pageSize",
124+
Typez: api.INT32_TYPE,
125+
},
126+
},
127+
}
128+
response := &api.Message{
129+
Name: "Response",
130+
ID: ".package.Response",
131+
Fields: []*api.Field{
132+
{
133+
Name: "next_page_token",
134+
JSONName: "nextPageToken",
135+
ID: ".package.Request.nextPageToken",
136+
Typez: api.STRING_TYPE,
137+
},
138+
{
139+
Name: "warnings",
140+
JSONName: "warnings",
141+
ID: ".package.Request.warnings",
142+
Typez: api.MESSAGE_TYPE,
143+
TypezID: ".package.Warning",
144+
Repeated: true,
145+
},
146+
{
147+
Name: "items",
148+
JSONName: "items",
149+
ID: ".package.Request.items",
150+
Typez: api.MESSAGE_TYPE,
151+
TypezID: ".package.Resource",
152+
Repeated: true,
153+
},
154+
},
155+
}
156+
method := &api.Method{
157+
Name: "List",
158+
ID: ".package.Service.List",
159+
InputTypeID: ".package.Request",
160+
OutputTypeID: ".package.Response",
161+
}
162+
service := &api.Service{
163+
Name: "Service",
164+
ID: ".package.Service",
165+
Methods: []*api.Method{method},
166+
}
167+
model := api.NewTestAPI([]*api.Message{request, response, resource}, []*api.Enum{}, []*api.Service{service})
168+
overrides := []config.PaginationOverride{
169+
{ID: ".package.Service.List", ItemField: "items"},
170+
}
171+
updateMethodPagination(overrides, model)
172+
if method.Pagination != request.Fields[1] {
173+
t.Errorf("mismatch, want=%v, got=%v", request.Fields[1], method.Pagination)
174+
}
175+
want := &api.PaginationInfo{
176+
NextPageToken: response.Fields[0],
177+
PageableItem: response.Fields[2],
178+
}
179+
if diff := cmp.Diff(want, response.Pagination); diff != "" {
180+
t.Errorf("mismatch, (-want, +got):\n%s", diff)
181+
}
182+
}
183+
98184
func TestPageMissingInputType(t *testing.T) {
99185
resource := &api.Message{
100186
Name: "Resource",
@@ -132,7 +218,7 @@ func TestPageMissingInputType(t *testing.T) {
132218
Methods: []*api.Method{method},
133219
}
134220
model := api.NewTestAPI([]*api.Message{response, resource}, []*api.Enum{}, []*api.Service{service})
135-
updateMethodPagination(model)
221+
updateMethodPagination(nil, model)
136222
if method.Pagination != nil {
137223
t.Errorf("mismatch, want=nil, got=%v", method.Pagination)
138224
}
@@ -179,7 +265,7 @@ func TestPageMissingOutputType(t *testing.T) {
179265
Methods: []*api.Method{method},
180266
}
181267
model := api.NewTestAPI([]*api.Message{request, resource}, []*api.Enum{}, []*api.Service{service})
182-
updateMethodPagination(model)
268+
updateMethodPagination(nil, model)
183269
if method.Pagination != nil {
184270
t.Errorf("mismatch, want=nil, got=%v", method.Pagination)
185271
}
@@ -227,7 +313,7 @@ func TestPageBadRequest(t *testing.T) {
227313
Methods: []*api.Method{method},
228314
}
229315
model := api.NewTestAPI([]*api.Message{request, response, resource}, []*api.Enum{}, []*api.Service{service})
230-
updateMethodPagination(model)
316+
updateMethodPagination(nil, model)
231317
if method.Pagination != nil {
232318
t.Errorf("mismatch, want=nil, got=%v", method.Pagination)
233319
}
@@ -279,7 +365,7 @@ func TestPageBadResponse(t *testing.T) {
279365
Methods: []*api.Method{method},
280366
}
281367
model := api.NewTestAPI([]*api.Message{request, response, resource}, []*api.Enum{}, []*api.Service{service})
282-
updateMethodPagination(model)
368+
updateMethodPagination(nil, model)
283369
if method.Pagination != nil {
284370
t.Errorf("mismatch, want=nil, got=%v", method.Pagination)
285371
}
@@ -444,21 +530,26 @@ func TestPaginationResponseErrors(t *testing.T) {
444530
}
445531

446532
for _, input := range []*api.Message{badToken, badItems, nil} {
447-
if got := paginationResponseInfo(input); got != nil {
533+
if got := paginationResponseInfo(nil, ".package.Service.List", input); got != nil {
448534
t.Errorf("expected paginationResponseInfo(...) == nil, got=%v, input=%v", got, input)
449535
}
450536
}
451537
}
452538

453539
func TestPaginationResponseItem(t *testing.T) {
540+
overrides := []config.PaginationOverride{
541+
{ID: ".package.Service.List", ItemField: "--invalid--"},
542+
}
454543
for _, test := range []struct {
455-
Name string
456-
Repeated bool
457-
Typez api.Typez
544+
Name string
545+
Repeated bool
546+
Typez api.Typez
547+
Overrides []config.PaginationOverride
458548
}{
459-
{"badRepeated", false, api.MESSAGE_TYPE},
460-
{"badType", true, api.STRING_TYPE},
461-
{"bothBad", false, api.ENUM_TYPE},
549+
{"badRepeated", false, api.MESSAGE_TYPE, nil},
550+
{"badType", true, api.STRING_TYPE, nil},
551+
{"bothBad", false, api.ENUM_TYPE, nil},
552+
{"badOverride", true, api.MESSAGE_TYPE, overrides},
462553
} {
463554
response := &api.Message{
464555
Name: "Response",
@@ -472,7 +563,7 @@ func TestPaginationResponseItem(t *testing.T) {
472563
},
473564
},
474565
}
475-
got := paginationResponseItem(response)
566+
got := paginationResponseItem(test.Overrides, ".package.Service.List", response)
476567
if got != nil {
477568
t.Errorf("the field should not be a pagination item, got=%v", got)
478569
}

0 commit comments

Comments
 (0)