Listable resource iteration through Iterator

[mgyucht]

Changes

A prototype of iterable functions for listable resources. The current Go SDK diverges from the Java and Python SDKs in how list operations are handled. The Go SDK synchronously exhaustively lists the resource (based on the given request) and returns a slice of all entries, and "limit" fields are interpreted as a global limit on the number of resources to return (i.e. the length of the returned slice is never longer than limit). In Python and Java, resource listing is done on demand by returning an iterator/generator abstraction from the list API immediately and only listing resources when requested. In these SDKs, the limit field is passed to the API and is only interpreted as the page size, so the number of returned resources is not bounded (though the number returned per REST API call is).

Besides this, in Go, there is not currently a common iterator abstraction. Even in the standard library, there are several different conventions used for iterator-like operations, such as (Scanner) Scan() (returning nil when scanning is complete). Without a common iterator interface, range expressions are not supported out of the box. See golang/go#54245 and golang/go#56413 for in-depth discussions about a proposed generic iterator interface in Go.

In light of all of this, this PR is a proposal for unifying the approaches taken between the Go and Python/Java SDKs to return an Iterator interface from listable APIs. This interface exposes Next(context.Context) (T, error) and HasNext(context.Context) methods that make it simple to iterate through the list of resources. This can be used for all endpoints using resource listing, including paginated and non-paginated endpoints alike. This reflects the implementation of the Java SDK and Java iterators, which also have next() and hasNext() methods.

Next returns the next object in the list, or if there are no more objects in the list, it returns the zero value and listing.ErrNoMoreValues. HasNext returns true if and only if there are more items to process, with one caveat: for some APIs (specifically offset-based APIs that don't include any sentinel value), it is not possible to determine whether there is a next item in the list response without actually making an API call. Because of this, HasNext must take a context.Context parameter. In case the API call fails, HasNext will return true, and the error can be retrieved by calling Next.

If an error occurs while making an API call, that error is cached and no further results are returned or fetched. In this case, the user is expected to call the List API once more to get a fresh iterator.

Other possible idioms

runners.CallersFrames returns a pointer to a named type *Frames with a method Next() (*Frames, bool). The boolean parameter indicates if there is another element available after the returned element. This mechanism does not support returning errors, though the error could be added as a third parameter.

bufio.Scanner provides 3 methods: Scan() bool indicating whether the scan has stopped (i.e. has reached EOF), Text() string the element returned by the last call to Scan(), and Err() error the error that occurred during scanning (unless the end of the file was reached). This is a slightly more cumbersome API requiring calls to all three methods separately in each loop, but Scan() can be used in a range expression.

database/sql.Rows provides a Next() bool to advance the result set to the next row and a Scan(dest ...any) error which scans the row into provided variables, returning an error if one occurred. This is suitable for a very generic endpoint (such as listing the results of a SQL query), but for most REST API calls where the type of the result is well known, it is less natural than simply returning a value.

What to do with the existing List API?

This raises a question of how to handle the existing *All methods. We can leave these *All methods in the SDK for convenience and because we have no need to break them now. However, in some sense, they increase the complexity of this project by having two ways to use an iterator, the first being listing.ToSlice(w.Service.List(ctx)) or listing.ToSliceN(w.Service.List(ctx), n), the second being w.Service.ListAll(ctx). The ListAll methods have an additional quirk that they use the Limit field in the request as the global limit for responses, which is different from our how other SDKs work. I think we can leave this here safely for now, but when the time comes for v1.0.0, we can remove these methods and encourage users to use the helper methods from the listing package directly. I don't feel the urgency to even mark these as deprecated at the moment, so we'll leave them as is.

Tests

• iteratorImpl and dedupeIteratorImpl are unit tested.

[pietern]

I like this approach; simple and to the point.

The WithLimit example you include would eventually return nil, nil even when the inner function keeps returning an error. If used as intended it would be fine, corner case.

One concern here is the use of the outer context for inner calls. Imagine iterating over a very large collection. The last pages still use the context (possibly with timeout) specified at the last call. What do you think of passing this context to the iterator? Then the timeout could apply to any single call as opposed to the end-to-end iteration.

[pietern]

Mix of := and var foo. Use of the same style is better, IMO.

Looking at the generated code, I was confused what finished meant because it doesn't break out. A more apt name would be along the lines of isLastPage, because results can still be non-empty.

[mgyucht]

err is uninitialized (gets its zero value) but finished is "initialized" to false. I thought it was more clear to set this explicitly this way rather than write var finished bool. Are you saying it's better to write

var err error
var finished bool

?

[pietern]

I was thinking of:

var finished = false

(also see other comment on name)

[mgyucht]

Yeah, I was having trouble naming that parameter. isLastPage is great, I'll use that.

[codecov-commenter]

Codecov Report

Attention: 1018 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
openapi/code/package.go	58.20% <100.00%> (ø)
service/compute/spark_version.go	0.00% <ø> (ø)
service/iam/model.go	0.00% <ø> (ø)
openapi/code/entity.go	26.88% <0.00%> (ø)
listing/listing.go	94.66% <94.66%> (ø)
openapi/code/method.go	32.76% <0.00%> (-1.76%)	⬇️
service/billing/api.go	0.00% <0.00%> (ø)
service/jobs/api.go	0.00% <0.00%> (ø)
service/pipelines/api.go	0.00% <0.00%> (ø)
service/settings/api.go	0.00% <0.00%> (ø)
... and 7 more

📢 Thoughts on this report? Let us know!.

[nfx]

What about ‘HasNext() bool’ + ‘Next() (T, error)’? This follows the idiomatic iteration from other SDKs. Client code is fluent enough then - for it.HasNext() { cluster, err := it.Next() if err != nil { return err } … } - technically, HasNext can be (bool, error), but we can hold on the error in the iteration state until Next() (T, error) gets called.

The similar way is used in azure and google SDKs. See azure key vault integration in deco tooling.

passing a closure makes the code less readable.

Limiting is to be done in the user code, if necessary. not sure if we need it in the SDK level.

Perfect solution would require constructing list request type without limit/offset/token/page fields and using that one.

re: backwards compatibility - XxxAll() ([]T, error) must stay and internally call these new methods. “Deprecated …” comment must be added to those.

[mgyucht]

Comparing to other clouds:

Azure: no resource-level iterator, but page-level iterator. Iterators have a type and define methods: (e.g. https://pkg.go.dev/github.com/azure/azure-sdk-for-go@v63.2.0+incompatible/services/databricks/mgmt/2018-04-01/databricks#VirtualNetworkPeeringListPage)
NextWithContext(context.Context) error loads the next page.
NotDone() bool is false if iteration hasn't started or there is a next page
Response() UnderlyingPage returns the raw response.
Values() []Items returns the items in the page.
I'm not sure we can implement this exactly because we may not know if there is a next page (only when the response includes next_page_token).

GCP: Iterators are a type and define methods: (e.g. https://pkg.go.dev/cloud.google.com/go/dataproc/apiv1#AutoscalingPolicyIterator.Next)
Next() (*Item, error) where error is a sentinel value if the item has been fully iterated
PageInfo() *PageInfo gives information about the current page, such as the token, max # of items per page, and # of remaining resources.

AWS: Returned result of a list operation is a page. Users are expected to iterate through the page on their own. They must check IsTruncated to see if there is a next page, and they must copy the next page token to the next request manually. The response includes the current page token as well. (e.g. https://pkg.go.dev/github.com/aws/aws-sdk-go/service/s3#ListBucketAnalyticsConfigurationsInput)

For us, it is not possible to write a HasNext() method or something like that without actually fetching the next field.

Using a typed response, rather than a closure, gives us more flexibility to add support for HasNext() in the future.

Exposing the underlying pages does mean there is slightly more work for the user to iterate through the result, but it also makes it very clear when a request is actually being made to get the next batch of responses.

Any approach needs to support all pagination methods we have today, which limits our options given that not all endpoints support page tokens.

type ListXResponse {
  // .. same fields as before, but also including other fields needed for pagination (not exported)
  nextPageToken string
  nextOffset string
}
func (*ListXResponse) NextPage(context.Context) (*ListXResponse, error)
// Later, when this can be determined by the response of a list operation
func (*ListXResponse) HasNextPage() bool
...
resp, err := w.Queries.List(ctx)
for len(resp.Values) == 0 {
  for _, q := range resp.Values {
    // do something
  }
  resp, err = resp.NextPage(ctx)
}

This is backwards-compatible with the current API and simply captures pagination state as unexported fields of the response.

[nfx]

For us, it is not possible to write a HasNext() method or something like that without actually fetching the next field.

@mgyucht why without actually fetching the next field is the requirement? Java/Python SDK does it.

Exposing the underlying pages

i'd want to block it for as long as possible. pagination implementations will evolve over time: jobs offset/limit -> cursor transition is the most recent example. what is in the page, how many items are in the page should be hidden from the average user. func (x *ListXResponse) NextPage(ctx) bool causes "active record" / "anemic data model" symptoms, which are very difficult to untangle over time, so we shouldn't do it.

some APIs have no pagination now, but will eventually get it in the near future. hence, consuming iterators/lists must have a consistent UX.

so I'll rephrase (reformat) my question again: why can't we

queries, err := w.Queries.List(ctx) // makes request, fills iterator state, catches failure. see Java impl
if err != nil {
  panic(err)
}
for queries.HasMore() {
  query, err := queries.Next()
  if err != nil {
    panic(err)
  }
  // do something
}

[nfx]

we must keep {{.PascalName}}All(...) ([]{{.Pagination.Entity.PascalName}}, error) method, as there's plenty of codebases that rely on this signature. our interface stability promise doesn't allow changing function signatures. at least without deprecating - see https://stackoverflow.com/questions/7849663/how-do-you-mark-code-as-deprecated-in-go for more info.

{{.PascalName}}All(...) ([]{{.Pagination.Entity.PascalName}}, error) must stay until v1.0.0

[nfx]

I have not seen this pattern (returning a callable function) anywhere in the go ecosystem, and it doesn't feel natural - there's nothing about a loop in the calling code.

How do we cross-compile examples of listing across different languages then? please add a jobs test without NoTranspile suffix and iterate over a couple of jobs, then regenerate the examples in go SDK. you'll have to modify openapi/code/roll package for that.

[nfx]

why do we need Iter suffix, if all SDKs already consistently return an iterator without suffixing by convention?

[nfx]

I wonder how would it look like if we port Java SDK's iteration implementation, moving the logic to a common class?

https://github.com/databricks/databricks-sdk-java/blob/main/databricks-sdk-java/src/main/java/com/databricks/sdk/support/Paginator.java#L14

[nfx]

if we're giving pagination a refactoring, then we should add a special request type for listing methods, that removes token, offset, and limit fields - so that the request has only the usable fields.

[nfx]

require.* halts the test execution, so this statement is unreachable

[mgyucht]

@mgyucht why without actually fetching the next field is the requirement? Java/Python SDK does it.

You need to specify a context when making an API call. What context would you use? The one used to make the original list request itself? That may be unnecessarily restrictive. Only when the request itself includes a next token will we know for certain that there are no more items to be returned by this list call. Java/Python don't have a context object to worry about.

pagination implementations will evolve over time: jobs offset/limit -> cursor transition is the most recent example.

The implementation of NextPage would hide the details of any of these transitions. Whether or not the fields are exposed is up to us, though we currently expose all of the fields of the response, even those related to pagination. That's why I like the NextPage() approach the most: users just call NextPage() when they need more results than contained in the current page.

what is in the page, how many items are in the page should be hidden from the average user.

Could you explain this a bit more? This is part of the public API, after all. Note that several APIs allow users to specify page size (like job listing). The only question is whether we should be abstracting over the fact that there may be pages.

func (x *ListXResponse) NextPage(ctx) bool causes "active record" / "anemic data model" symptoms, which are very difficult to untangle over time, so we shouldn't do it.

I was trying to understand this but wasn't following. From my reading, anemic domain model means that code that acts on objects is decoupled from the object itself and is embedded in the program architecture. In this case, though, the NextPage implementation would ensure that each structure is synthesized with its own pagination logic.

some APIs have no pagination now, but will eventually get it in the near future. hence, consuming iterators/lists must have a consistent UX.

I agree, and I think this does provide a consistent UX. For non-paginated APIs, the NextPage method would just return nil, nil for now, as all of the results would be present in the List...Response.

[pietern]

Why not return Iterator[T], this being a public function?

[mgyucht]

I think it's kind of up to users if they want to define interfaces, right? We define the interface so that we can provide this to NewDedupeIterator, but if we add more public methods to this base type, users should in theory be able to use them.

[pietern]

It's a unexported type though, so for documentation sake I guess it would be better to document it as returning an Iterator[T]. As is, it will show up as unexported type.

[mgyucht]

Actually, I was thinking about this a bit more, and I think we may want to export this type as well and have the List implementations return the actual concrete type. None of the fields will be exported, so users won't be able to modify iterator state, but e.g. if we define more methods on either of those structures that don't make sense for the overall iterator (e.g. imagine a "GetCached" on DedupeIteratorImpl that returns the cached element or nil), we couldn't expose that without changing the Iterator interface or using the concrete type.

[pietern]

This logic is a bit weird now. Limits should be applied at the iterator level, as these control the per-page limit.

[mgyucht]

I'm trying to preserve the current behavior, do you think I did something wrong?

Eventually, I would like to remove all pagination parameters from request structures so that users don't misuse them or get confused by them anymore.

[pietern]

No, it persist existing behavior. Just saying there is ambiguity in this approach; i.e. if I specify limit, does it apply to the number of elements per page, or the total number of elements returned.

[mgyucht]

Agreed, it's very confusing. I think we made a change earlier to ensure that limit applied to the total returned number of elements when using the SDK, but that diverges from the REST API behavior (as well as from other SDK's behaviors). I do eventually want to remove these parameters from the list request structure, as well as all pagination-based parameters (for example, when using the SDK, why should a user be able to specify the token to use?).

Changes have been addressed.