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 5 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
16 changes: 10 additions & 6 deletions .codegen/api.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,11 @@ func (a *{{.Service.Name}}API) {{.PascalName}}AndWait(ctx context.Context{{if .R
{{.Comment "// " 80}}
//
// 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
func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .ListingRequest}}, listing {{.ListingRequest.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 .ListingRequest}}request := {{.Request.PascalName}}{ {{range .ListingRequest.Fields }}
{{.PascalName}}: listing.{{.PascalName}},{{end}}
}{{end}}
{{ if .Pagination.Limit -}}
var totalCount {{template "type" .Pagination.Limit.Entity}} = 0
{{ end -}}
Expand Down Expand Up @@ -203,6 +206,7 @@ func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .Reque
if response.NextPage == nil {
break
}
// cluster events API returns next page information as part of the result
request = *response.NextPage
{{- else if .Pagination.Token -}}
request.{{.Pagination.Token.PollField.PascalName}} = response.{{.Pagination.Token.Bind.PascalName}}
Expand All @@ -221,11 +225,11 @@ func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .Reque
}
{{- end -}}
}
return results, nil{{else if .Pagination.Results}}response, err := a.impl.{{.PascalName}}(ctx{{if .Request}}, request{{end}})
return results, nil{{else if .Pagination.Results}}response, err := a.impl.{{.PascalName}}(ctx{{if .Request}}, listing{{end}})
if err != nil {
return nil, err
}
return response.{{.Pagination.Results.PascalName}}, nil{{else}}return a.impl.{{.PascalName}}(ctx, request){{end}}
return response.{{.Pagination.Results.PascalName}}, nil{{else}}return a.impl.{{.PascalName}}(ctx, listing){{end}}
}
{{end}}{{if .NamedIdMap}}
// {{.NamedIdMap.PascalName}} calls [{{.Service.Name}}API.{{.PascalName}}{{if not .NamedIdMap.Direct}}All{{end -}}] and creates a map of results with [{{.NamedIdMap.Entity.PascalName}}]{{ template "field-path" .NamedIdMap.NamePath }} as key and [{{.NamedIdMap.Entity.PascalName}}]{{ template "field-path" .NamedIdMap.IdPath}} as value.
Expand All @@ -235,10 +239,10 @@ func (a *{{.Service.Name}}API) {{.PascalName}}All(ctx context.Context{{if .Reque
// Note: All [{{.NamedIdMap.Entity.PascalName}}] instances are loaded into memory before creating a map.
//
// This method is generated by Databricks SDK Code Generator.
func (a *{{.Service.Name}}API) {{.NamedIdMap.PascalName}}(ctx context.Context{{if .Request}}, request {{.Request.PascalName}}{{end}}) (map[string]{{template "type" .NamedIdMap.Id.Entity}}, error) {
func (a *{{.Service.Name}}API) {{.NamedIdMap.PascalName}}(ctx context.Context{{if .ListingRequest}}, listing {{.ListingRequest.PascalName}}{{end}}) (map[string]{{template "type" .NamedIdMap.Id.Entity}}, error) {
ctx = useragent.InContext(ctx, "sdk-feature", "name-to-id")
mapping := map[string]{{template "type" .NamedIdMap.Id.Entity}}{}
result, err := a.{{.PascalName}}{{if not .NamedIdMap.Direct}}All{{end}}(ctx{{if .Request}}, request{{end}})
result, err := a.{{.PascalName}}{{if not .NamedIdMap.Direct}}All{{end}}(ctx{{if .ListingRequest}}, listing{{end}})
if err != nil {
return nil, err
}
Expand All @@ -262,7 +266,7 @@ func (a *{{.Service.Name}}API) {{.NamedIdMap.PascalName}}(ctx context.Context{{i
// This method is generated by Databricks SDK Code Generator.
func (a *{{.Service.Name}}API) GetBy{{range .NamedIdMap.NamePath}}{{.PascalName}}{{end}}(ctx context.Context, name string) (*{{template "type" .GetByName}}, error) {
ctx = useragent.InContext(ctx, "sdk-feature", "get-by-name")
result, err := a.{{.PascalName}}{{if not .NamedIdMap.Direct}}All{{end}}(ctx{{if .Request}}, {{.Request.PascalName}}{}{{end}})
result, err := a.{{.PascalName}}{{if not .NamedIdMap.Direct}}All{{end}}(ctx{{if .ListingRequest}}, {{.ListingRequest.PascalName}}{}{{end}})
if err != nil {
return nil, err
}
Expand Down
2 changes: 2 additions & 0 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
ListingRequest *Entity
// Response type representation
Response *Entity
EmptyResponseName Named
Expand Down
78 changes: 78 additions & 0 deletions openapi/code/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ func (svc *Service) newMethod(verb, path string, params []openapi.Parameter, op
Verb: strings.ToUpper(verb),
Path: path,
Request: request,
ListingRequest: svc.withPaginationFieldsRemoved(request, op.Pagination),
nfx marked this conversation as resolved.
Show resolved Hide resolved
PathParts: svc.paramPath(path, request, params),
Response: response,
EmptyResponseName: emptyResponse,
Expand All @@ -317,6 +318,83 @@ 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 {
if req == nil {
return nil
}
if svc.Name == "Clusters" && req.CamelName() == "getEvents" {
// edge case: cluster events performs listing through POST, not GET
nfx marked this conversation as resolved.
Show resolved Hide resolved
// all other listing requests entities are synthesized.
return req
}
if pg == nil || pg.Inline {
return req
}
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 {
// there was no change. A bit strange.
return req
nfx marked this conversation as resolved.
Show resolved Hide resolved
}
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
}
oldName := req.Name
_, needsRename := svc.Package.types[oldName]
if needsRename {
newName := strings.TrimSuffix(req.PascalName(), "Request") + "Internal"
nfx marked this conversation as resolved.
Show resolved Hide resolved
req.Name = newName
req.Description = "For internal use only"
delete(svc.Package.types, oldName)
svc.Package.types[newName] = req
}
return svc.Package.define(listing)
}

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

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

48 changes: 30 additions & 18 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.

Loading