Common design for PaginatedResult-related methods

[ably-sync-bot]

Continue this discussion.

[tcard]

Replying to @lmars at #206 (comment):

The usage feels a little awkward, I wonder if something like the following would be better:

params := &ably.PaginateParams{
	Limit: 10,
}
err := channel.Presence.Get(params, func(page *ably.PaginatedResult) bool {
	// iterate page.PresenceMessages()
	// return false to stop iterating
})

and similarly for the other paginated operations.

This has a number of issues:

• A generic PaginateParams wouldn't fit all paginated methods. It would be either a common subset or a superset of all, some of them meaningless depending on the method.
• Iterating by
• A generic PaginatedResult wouldn't be type-safe: you could call .PresenceMessages() on a page containing Stats, for example.
• Functions for iteration ("internal iterators" as defined here) is less expressive since the user isn't in charge of control flow (see previously linked article). Also, it's just not that idiomatic.

So, here's my proposal.

Idiomatic PaginatedResult

The specified API for PaginatedResult is:

class PaginatedResult<T>:
  items: [T] // TG3
  first() => io PaginatedResult<T> // TG5
  hasNext() -> Bool // TG6
  isLast() -> Bool // TG7
  next() => io PaginatedResult<T>? // TG4

A direct translation to Go would be:

type PaginatedResult[T] interface {
	Items() []T
	First(context.Context) (PaginatedResult[T], error)
	HasNext() bool
	IsLast() bool
	Next(context.Context) (PaginatedResult[T], error) // io.EOF-like error
}

If you just wanted to iterate all items from all pages, you'd use it maybe like:

for r, err := rest.Foo(ctx, ...); err == nil; r, err = r.Next(ctx) {
	for _, item := range r.Items() {
		...
	}
}
if !errors.Is(err, ably.EndOfPages) { // or similar
	...
}

This is pretty awkward. I think something like this would be more idiomatic:

// Explicitly going through each page.
pages := rest.Foo(...)
for pages.Next(ctx) {
	for _, item := range pages.Items() {
		...
	}
}
if err := r.Err(); err != nil {
	...
}

// Paging implicit; just iterate over items.
items := rest.Foo(...).Items()
for items.Next(ctx) {
	item := items.Item()
	...
}
if err := r.Err(); err != nil {
	...
}

This is more in line with the stdlib, e. g. database/sql#DB.Query, bufio#Scanner.

Generics, or lack thereof

So, obviously, another issue with the above is that Go doesn't have generics (yet).

We actually have a couple precedents for generic types: EventEmitter (Channel, Connection) and Channels (Realtime, REST). So I think we should do the same for all instances of PaginatedResult: have separate types for each with the same interface, except for the item type. That is: Stats, Message, PresenceMessage, and JsonObject (this one embedded in in HTTPPaginatedResponse, and which I'd just leave as json.RawMessage).

Query parameters

Spec:

(Rest, Realtime).request(
	String method,
	String path,
	Dict<String, String> params?,
	JsonObject | JsonArray body?,
	Dict<String, String> headers
) => io HttpPaginatedResponse // RSC19

(Rest, Realtime).stats(
	start: Time api-default epoch(), // RSC6b1
	end: Time api-default now(), // RSC6b1
	direction: .Backwards | .Forwards api-default .Backwards, // RSC6b2
	limit: int api-default 100, // RSC6b3
	unit: .Minute | .Hour | .Day | .Month api-default .Minute // RSC6b4
) => io PaginatedResult<Stats> // RSC6a

Channel.history(
	start: Time, // RSL2b1
	end: Time api-default now(), // RSL2b1
	direction: .Backwards | .Forwards api-default .Backwards, // RSL2b2
	limit: int api-default 100 // RSL2b3

	// Only in RealtimeChannel:
	untilAttach: Bool default false // RTL10b
) => io PaginatedResult<Message> // RSL2a

Presence.history(
	start: Time, // RSP4b1
	end: Time api-default now(), // RSP4b1
	direction: .Backwards | .Forwards api-default .Backwards, // RSP4b2
	limit: int api-default 100, // RSP4b3
) => io PaginatedResult<PresenceMessage> // RSP4a

Presence.get(
	limit: int api-default 100, // RSP3a
	clientId: String?, // RSP3a2
	connectionId: String?, // RSP3a3
) => io PaginatedResult<PresenceMessage> // RSPa

Given we have mostly optional parameters, we should follow the optional parameters pattern as defined in #197 (example).

Each method would have its own <Method>Option function type. Even if we end up having multiple identical limit, start, end, etc. options, this ensures forwards compatibility if in the future some parameters get added to one of the methods but not to another.

---

@lmars @sacOO7 PTAL

[lmars]

@tcard this is great! I support this, the only thing I would say is that I think the initial call should return an error. So rather than:

pages := rest.Foo(...)

it would be:

pages, err := rest.Foo(...)

Reasons for this:

• if there is going to be an error, it's most likely going to come from the initial call, so makes sense to handle it up front

• it reduces the chance of the error being ignored since it is very easy to do pages := rest.Foo() then iterate through with pages.Next() but forget to check pages.Err() at the end

• it's what database/sql#DB.Query does

[tcard]

That sounds reasonable, the counterargument being that bufio.Scanner does the opposite and that having to check the error twice is more verbose and maybe makes it more likely that the second check is ignored. But no strong preferences either way.