Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve semantics for listing methods #569

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions .codegen/api.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,9 @@ func (a *{{.Service.Name}}API) {{.PascalName}}AndWait(ctx context.Context{{if .R
// This method is generated by Databricks SDK Code Generator.
func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .Request}}, request {{.Request.PascalName}}{{end}}) ([]{{.Pagination.Entity.PascalName}}, error) {
nfx marked this conversation as resolved.
Show resolved Hide resolved
{{if .Pagination.MultiRequest}}var results []{{.Pagination.Entity.PascalName}}
{{if .RawRequest}}listing := {{.RawRequest.PascalName}}{ {{range .Request.Fields }}
{{.PascalName}}: request.{{.PascalName}},{{end}}
}{{end}}
{{ if .Pagination.Limit -}}
var totalCount {{template "type" .Pagination.Limit.Entity}} = 0
{{ end -}}
Expand All @@ -172,9 +175,9 @@ func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .Reque
// deduplicate items that may have been added during iteration
seen := map[{{template "type" .IdentifierField.Entity}}]bool{}
{{end}}{{if eq .Pagination.Increment 1 -}}
request.{{.Pagination.Offset.PascalName}} = 1 // start iterating from the first page
listing.{{.Pagination.Offset.PascalName}} = 1 // start iterating from the first page
{{end}}for {
response, err := a.impl.{{.PascalName}}(ctx{{if .Request}}, request{{end}})
response, err := a.impl.{{.PascalName}}(ctx{{if .Request}}, listing{{end}})
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -203,19 +206,20 @@ func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .Reque
if response.NextPage == nil {
break
}
request = *response.NextPage
// cluster events API returns next page information as part of the result
listing = *response.NextPage
{{- else if .Pagination.Token -}}
request.{{.Pagination.Token.PollField.PascalName}} = response.{{.Pagination.Token.Bind.PascalName}}
listing.{{.Pagination.Token.PollField.PascalName}} = response.{{.Pagination.Token.Bind.PascalName}}
if response.{{.Pagination.Token.Bind.PascalName}} == "" {
break
}
{{- else if eq .Pagination.Increment 1 -}}
request.{{.Pagination.Offset.PascalName}}++
listing.{{.Pagination.Offset.PascalName}}++
{{- else -}}
request.{{.Pagination.Offset.PascalName}} += {{template "type" .Pagination.Offset.Entity}}(len(response.{{.Pagination.Results.PascalName}}))
listing.{{.Pagination.Offset.PascalName}} += {{template "type" .Pagination.Offset.Entity}}(len(response.{{.Pagination.Results.PascalName}}))
{{- end}}
{{ if .Pagination.Limit -}}
limit := request.{{.Pagination.Limit.PascalName}}
limit := listing.{{.Pagination.Limit.PascalName}}
if limit > 0 && totalCount >= limit {
break
}
Expand Down Expand Up @@ -283,7 +287,7 @@ func (a *{{.Service.Name}}API) GetBy{{range .NamedIdMap.NamePath}}{{.PascalName}
{{end}}{{if .Shortcut}}
{{.Comment "// " 80}}
func (a *{{.Service.Name}}API) {{.Shortcut.PascalName}}(ctx context.Context{{range .Shortcut.Params}}, {{.CamelName}} {{template "type" .Entity}}{{end}}) {{if .Response}}({{if .Response.ArrayValue}}[]{{.Response.ArrayValue.PascalName}}{{else}}*{{template "type" .Response}}{{end}}, error){{else}}error{{end}} {
return a.impl.{{.PascalName}}(ctx, {{.Request.PascalName}}{
return a.impl.{{.PascalName}}(ctx, {{.RealRequest.PascalName}}{
{{- range .Shortcut.Params}}
{{.PascalName}}: {{.CamelName}},{{end}}
})
Expand Down
4 changes: 2 additions & 2 deletions .codegen/impl.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ type {{.CamelName}}Impl struct {
}

{{range .Methods}}
func (a *{{.Service.CamelName}}Impl) {{.PascalName}}(ctx context.Context{{if .Request}}, request {{.Request.PascalName}}{{end}}) {{if .Response}}({{if .Response.ArrayValue}}[]{{.Response.ArrayValue.PascalName}}{{else}}*{{template "type" .Response}}{{end}}, error){{else}}error{{end}} {
func (a *{{.Service.CamelName}}Impl) {{.PascalName}}(ctx context.Context{{if .RealRequest}}, request {{.RealRequest.PascalName}}{{end}}) {{if .Response}}({{if .Response.ArrayValue}}[]{{.Response.ArrayValue.PascalName}}{{else}}*{{template "type" .Response}}{{end}}, error){{else}}error{{end}} {
{{if .Response}}var {{.Response.CamelName}} {{if .Response.ArrayValue}}[]{{.Response.ArrayValue.PascalName}}{{else}}{{template "type" .Response}}{{end}}
{{end -}}
path := {{if .PathParts -}}
fmt.Sprintf("{{range .PathParts}}{{.Prefix}}{{if or .Field .IsAccountId}}%v{{end}}{{ end }}"{{ range .PathParts }}{{if .Field}}, request.{{.Field.PascalName}}{{ else if .IsAccountId }}, a.client.ConfiguredAccountID(){{end}}{{ end }})
{{- else}}"{{.Path}}"{{end}}
err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}})
err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, {{if .RealRequest}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}})
return {{if .Response}}{{if not .Response.ArrayValue}}&{{end}}{{.Response.CamelName}}, {{end}}err
}
{{end -}}
Expand Down
2 changes: 1 addition & 1 deletion .codegen/interface.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ type {{.PascalName}}Service interface {
//
// Use {{.PascalName}}All() to get all {{.Pagination.Entity.PascalName}} instances{{if .Pagination.MultiRequest}}, which will iterate over every result page.{{end}}
{{- end}}
{{.PascalName}}(ctx context.Context{{if .Request}}, request {{.Request.PascalName}}{{end}}) {{if .Response}}({{if .Response.ArrayValue}}[]{{.Response.ArrayValue.PascalName}}{{else}}*{{template "type" .Response}}{{end}}, error){{else}}error{{end}}
{{.PascalName}}(ctx context.Context{{if .RealRequest}}, request {{.RealRequest.PascalName}}{{end}}) {{if .Response}}({{if .Response.ArrayValue}}[]{{.Response.ArrayValue.PascalName}}{{else}}*{{template "type" .Response}}{{end}}, error){{else}}error{{end}}
{{end -}}
}

Expand Down
17 changes: 14 additions & 3 deletions openapi/code/method.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ type Method struct {
PathParts []PathPart
// Request type representation
Request *Entity
// Request for listing
RawRequest *Entity
// Response type representation
Response *Entity
EmptyResponseName Named
Expand Down Expand Up @@ -209,6 +211,15 @@ func (m *Method) Wait() *Wait {
}
}

// Returns either RawRequest used for fetching individual pages of data
// or a more friendly request, if it's there.
func (m *Method) RealRequest() *Entity {
if m.RawRequest != nil {
return m.RawRequest
}
return m.Request
}

// Pagination returns definition for possibly multi-request result iterator
func (m *Method) Pagination() *Pagination {
if m.pagination == nil {
Expand All @@ -221,12 +232,12 @@ func (m *Method) Pagination() *Pagination {
var token *Binding
if m.pagination.Token != nil {
token = &Binding{ // reuse the same datastructure as for waiters
PollField: m.Request.Field(m.pagination.Token.Request),
PollField: m.RawRequest.Field(m.pagination.Token.Request),
Bind: m.Response.Field(m.pagination.Token.Response),
}
}
offset := m.Request.Field(m.pagination.Offset)
limit := m.Request.Field(m.pagination.Limit)
offset := m.RawRequest.Field(m.pagination.Offset)
limit := m.RawRequest.Field(m.pagination.Limit)
results := m.Response.Field(m.pagination.Results)
if results == nil {
panic(fmt.Errorf("invalid results field '%v': %s",
Expand Down
85 changes: 85 additions & 0 deletions openapi/code/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,14 @@ func (svc *Service) newMethod(verb, path string, params []openapi.Parameter, op
}
idFieldPath = idField
}
request, listingRequest := svc.withPaginationFieldsRemoved(request, op.Pagination)
return &Method{
Named: Named{name, description},
Service: svc,
Verb: strings.ToUpper(verb),
Path: path,
Request: request,
RawRequest: listingRequest,
PathParts: svc.paramPath(path, request, params),
Response: response,
EmptyResponseName: emptyResponse,
Expand All @@ -317,6 +319,89 @@ func (svc *Service) newMethod(verb, path string, params []openapi.Parameter, op
}
}

// remove pagination fields without clear semantics for iterators
func (svc *Service) withPaginationFieldsRemoved(req *Entity, pg *openapi.Pagination) (*Entity, *Entity) {
if req == nil {
return nil, nil
}
if svc.Name == "Clusters" && req.CamelName() == "getEvents" {
// edge case: cluster events performs listing through POST, not GET.
// all other listing requests entities are synthesized.
return req, req
}
if pg == nil || pg.Inline {
return req, nil
}
if pg.Offset == "" &&
pg.Limit == "" &&
pg.Increment == 0 &&
pg.Token == nil &&
pg.Results != "" {
return req, nil
}
listing := &Entity{
Named: Named{
Name: req.PascalName(),
Description: req.Description,
},
Package: req.Package,
RequiredOrder: req.RequiredOrder,
fields: map[string]*Field{},
}
var requiresModification bool
for _, v := range req.fields {
field := v
var nextToken string
if pg.Token != nil {
nextToken = pg.Token.Request
}
switch field.Name {
case pg.Limit, pg.Offset, nextToken:
requiresModification = true
continue
}
listing.fields[field.Name] = field
}
if !requiresModification {
panic(fmt.Errorf("incorrect type detected: %s", req.FullName()))
}
if svc.Name == "Jobs" {
// add generation for client-side maximum number of results during iteration.
// platform already implements this field for part of the APIs and it's
// consistently named as max_results everywhere. Perhaps we should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_results is also confusing and incorrect as listing will return all results, regardless of the value passed here. Can we rename this field to page_size instead? This aligns with our internal standards on pagination.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it we use any other name than limit, it'll break the CLI. @pietern are we fine with that? :)

max_results is also confusing and incorrect as listing will return all results, regardless of the value passed here.

we can make deterministic logic here. max_results is really the closest thing semantically. page_size is not applicable here, as this listing request is for the entire dataset, not the page. the concept of page is totally hidden from a user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the concept of page is hidden from users, then we should not expose this parameter at all, as its only use is to determine the page size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're keeping this limit for CLI, then. other renames are out of scope for now.

// rename "limit" to "max_results" in Jobs and add that to Dashboards and
// Queries as well, as we have exactly the same problem on big workspaces.
pg.Limit = "limit"
listing.fields["limit"] = &Field{
Named: Named{
Name: "limit",
Description: "limit maximum number of results on the client side",
},
Of: listing,
IsJson: false,
IsQuery: false,
IsPath: false,
Entity: &Entity{
IsInt: true,
},
}
}
if len(listing.fields) == 0 {
// there is no fields left.
return nil, req
}
oldName := req.Name
_, needsRename := svc.Package.types[oldName]
if needsRename {
newName := strings.TrimSuffix(req.CamelName(), "Request") + "Internal"
req.Name = newName
req.Description = "For internal use only"
delete(svc.Package.types, oldName)
svc.Package.types[newName] = req
}
return svc.Package.define(listing), req
}

func (svc *Service) HasWaits() bool {
for _, v := range svc.methods {
if v.wait != nil {
Expand Down
20 changes: 16 additions & 4 deletions service/catalog/api.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions service/catalog/impl.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions service/catalog/interface.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 35 additions & 4 deletions service/catalog/model.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading