PaginatedResource API change

[mattheworiordan]

Whilst our client APIs are consistent, the PaginatedResource API has unfortunately diverged somewhat. Following some discussions, we have decided to implement the PaginatedResource so that it is iterator-like, provides an accessor / method to the items it holds to encourage understanding of the pages of results concept to developers i.e. each next page is a new request for new items, and provides similar synchronous and asynchronous interfaces.

The proposed interface demonstrated in code is therefore as follows:

Async Javascript

function displayPages(currentPage) {
    for (let message in currentPage.items) {
      console.log(message.name);
    }
    if (let next = currentPage.next) {
      next(function(err, nextPage) { displayPages(nextPage); });
    }
}

channel.history(function (err, currentPage) {
  currentPage.isFirst(); // true
  currentPage.isLast(); // false
  displayPages(currentPage);
}

Sync Ruby

current_page = channel.history

current_page.first? # true
current_page.last? # false
current_page.has_next? # true

do 
  current_page.items.each { |message| puts message.name }
  current_page = current_page.next
until current_page.last?

current_page.next # => nil

[rjeczalik]

Go

it := presence.Get(params)
for it.Next() {
    page := it.PresenceMessages()
    for _, item := range page {
        fmt.Println(item)
    }
}
if err := it.Err(); err != nil {
    panic(err)
}

[mattheworiordan]

@rjeczalik so that looks good although I think it is wrong because a) it.Next() will be called in the condition check when the for loop starts advancing the user to the next page, b) we wanted to generalise the access to the items within a paginated resource so that the developer does not necessarily specify the type, c) whilst it's iterator like, it's not an iterator and as such Next() returns a new PaginatedResource, and does not advance (we chose this design because it's more portable across languages, especially where async is required, and also means users can explicitly keep a reference to a page if they want). I appreciate you may in fact need to create typed versions of the PaginatedResource, however if you do that, we would still like to keep the API consistent so that in future we can use PaginatedResource for any type of resource, shame there aren't generics in Go.

I expect it will therefore look more like this unfortunately:

currentPage, err := presence.Get(params)
for {
    if err != nil {
        panic(err)
    }
    if currentPage == nil {
        break
    }
    for _, item := range currentPage.items {
        fmt.Println(item)
    }
    currentPage, err = currentPage.Next()
}

With my example I am not sure how the page would return an error though, you may need to advise on the best way of doing this.

[rjeczalik]

(in advance sorry for lengthy post and no TL;DR)

@mattheworiordan Depends what our priorities are:

1. if stick to cross-language interface of PaginatedResource then current implementation already does look like your code snippet, eventually it may rearranged to the following to simplify error handling:

page, err := presence.Get(params)
for ; err == nil; page, err = page.Next() {
    for _, item := range page.Items() {
        fmt.Println(item)
    }
}
if err != nil {
    panic(err)
}

2. if idiomatic Go, we would need to eliminate excessive error checking there

In order to make it idiomatic, the most important difference is to store the error within the currentPage and loop over Next() bool, which returns true when page was fetched successfully, and false where either there's no more pages or an error occurred.

And addressing questions:

a) it.Next() will be called in the condition check when the for loop starts advancing the user to the next page

On the first iteration it.Next() actually will fetch the first page, since the iterator returned by params.Get(params) has no pages fetched, it only builds iterator (or PaginatedResource).

b) we wanted to generalise the access to the items within a paginated resource so that the developer does not necessarily specify the type

Yup, the Go PaginatedResource or Iterator (btw should we stick to the former and name the iterator type PaginatedResource?) will have Items() []interface{} function to return all methods. This would not be possible if we stick with current implementation described in 1). Complementary to generic Items() it would also have methods like Messages() []*Message, Stats() []*Stat etc. for convenience casting.

c) whilst it's iterator like, it's not an iterator and as such Next() returns a new PaginatedResource, and does not advance (we chose this design because it's more portable across languages, especially where async is required, and also means users can explicitly keep a reference to a page if they want).

Yup, the way I intend the iterator to work is to mutate itself to the next page, so currentPage is always newly fetched page, and there's no advancements. I'm not sure for what particularly user may want to keep the reference, I see two use-cases:

• if user wants to keep reference to current page, it's as simple as storing items from that page:

var i, itemsFrom3rdPage = 0, []interface{}(nil)
for page.Next() {
    items := page.Items()
    if i++; i == 3 {
       itemsFrom3rdPage = items
    }
    // as items does not mutate between iterations, it can be handled async
    go fmt.Println(items)
}
if err := page.Err(); err != nil {
    panic(err)
}
fmt.Println(itemsFrom3rdPage)

• if reference to the current page is needed for the purpose of re-running the iteration later on from particular point, it is possible by copying the iterator explicitely:

var i, page3rd = 0, (*ably.PaginatedResource)(nil)
for page.Next() {
    items := page.Items()
    if i++; i == 3 {
       page3rd = page.Ref() // or page.Clone()
    }
    // as items does not mutate between iterations, it can be handled async
    go fmt.Println(items)
}
if err := page.Err(); err != nil {
    panic(err)
}
for page3rd.Next() {
   // continue ranging after 3rd page
}

@mattheworiordan @kouno The call is not mine, let me know guys which one, 1) or 2) you prefer to have.

[paddybyers]

if reference to the current page is needed for the purpose of re-running the iteration later on from particular point

When a query is performed, various of the params are typically unspecified, but are then resolved to specific values when the query is evaluated; eg start, end times are resolved to specific times. The current params for a page contain those resolved param values, so that page of results can be repeatably fetched. That isn't possible just by re-running the original query with the original params.

[rjeczalik]

@paddybyers Yup, the page3rd already contains resolved params as the page.Ref/Copy() just returns copy of the page after query (it must be separate method, as page needs to copy its unexported pointer members and simply page3rd := *page won't work).

After rethinking this I withdrawn my statement that my proposed second approach is more idiomatic than the first one - traversing the pages as they were nodes is also fine. I'd just remove the wrappers that each function in rest / realtime client wraps PaginatedResource, make a single generic version and use for every method it requires. Sounds reasonable @mattheworiordan @kouno?

[kouno]

👍 sounds awesome.

[mattheworiordan]

@rjeczalik sounds reasonable, but I think it's important that we don't make PaginatedResource an iterator to iterate through pages. Other languages for various reasons can't really do that, especially Javascript, so we'd prefer to keep the behaviour as similar as possible between languages, so Next() or First() should return a PaginatedResource with the Items. Is that OK?

[rjeczalik]

@mattheworiordan Yup, crystal clear, I settled on exactly that.

[mattheworiordan]

@paddybyers, @rjeczalik and @kouno, I have implemented this feature in the Ruby API as I wanted to see what was involved. It didn't take very long, see https://github.com/ably/ably-ruby/tree/new-paginated-resource. I find the API is now a bit more verbose, but equally I think conceptually it's a lot clearer what developers are dealing with when accessing paged results. See https://github.com/ably/ably-ruby/pull/26/files#diff-04c6e90faac2675aa89e2176d2eec7d8 for a description of how it's changed in the Readme. What do you think?

[thevixac]

I think the iOS pagination is what you want. You get a ARTPaginatedResult back,
which is essentially an iterator. You can ask it for the first, current, or next page.
Each time you ask for the next page, you get a new ARTPaginatedResult object. ARTPaginatedResult is used for all paginated queries regardless of payload type.

[channel historyWithParams:@{@"limit" : @"2",
   @"direction" : @"backwards"} cb:^(ARTStatus status, id<ARTPaginatedResult> result) {
       XCTAssertEqual(status, ARTStatusOk);
       XCTAssertTrue([result hasFirst]);
       XCTAssertTrue([result hasNext]);
       NSArray * page = [result current];
       XCTAssertEqual([page count], 2);
       ARTMessage * firstMessage = [page objectAtIndex:0];
       ARTMessage * secondMessage =[page objectAtIndex:1];
       XCTAssertEqualObjects(@"testString4", [firstMessage content]);
       XCTAssertEqualObjects(@"testString3", [secondMessage content]);
       [result getNext:^(ARTStatus status, id<ARTPaginatedResult> result2) {
           XCTAssertEqual(status, ARTStatusOk);
           XCTAssertTrue([result2 hasFirst]);
           NSArray * page = [result2 current];
           XCTAssertEqual([page count], 2);
           ARTMessage * firstMessage = [page objectAtIndex:0];
           ARTMessage * secondMessage =[page objectAtIndex:1];

           XCTAssertEqualObjects(@"testString2", [firstMessage content]);
           XCTAssertEqualObjects(@"testString1", [secondMessage content]);

           [result2 getNext:^(ARTStatus status, id<ARTPaginatedResult> result3) {
                      //do something with result3
           }];
       }];
   }];

[mattheworiordan]

@thevixac thanks for the post. I think the iOS API looks very close in fact. However, the key difference is that instead of providing the attribute count on the ARTPaginatedResult class, count should be available as an option on the #items Array attribute. Making it clear to a developer they are dealing with a page is good and the iOS library is good in that regard. However, exposing count as an attribute could be confusing because is that a count of pages or items held within the page. Hence we would like all Messages/Stats/PresenceMessages to be available from the #items attribute, and any query to see how many items are available should be directed to the items attribute itself.

[paddybyers]

I've got two alternative implementations in javascript.

The first uses the fact that there is a callback that can return multiple results to return the items and the rel links in two separate callback arguments:

channel.history(<params>, function(err, messages, links) {

  // messages is an array of messages
  var messageCount = messages.length;
  var firstMessage = messages[0];

  // links contains functions to perform the query to get the related page
  var hasNext = !!links.next;
  links.next(function(err, messages, links) { ... });
});

The second API more closely follows the generic proposal, where a single ResultPage result is returned, and it has items and the applicable links directly available as methods:

channel.history(<params>, function(err, resultPage) {

  // resultPage.items is an array of messages
  var messageCount = resultPage.items.length;
  var firstMessage = resultPage.items[0];

  // resultPage contains functions to perform the query to get the related page
  var hasNext = !!resultPage.next;
  resultPage.next(function(err, nextPage) { ... });
});

[mattheworiordan]

Given the second is closer to other implementations I think that is best. Are you looking for confirmation before one is implemented?

[paddybyers]

I've implemented both, but I prefer the first :)

[mattheworiordan]

Ah, ok, thanks.

[mattheworiordan]

I don't think this is working unfortunately.

Here is some code I am using to get history:

channel.history(limit: 1000, function (err, messages) {
  console.log(err); // => null
  console.log(messages.items); // => null
  console.log(messages.length); // => 2
});

Is the common API active? It seems like it is defaulting to the channel.history(<params>, function(err, messages, links) { API

[paddybyers]

Is the common API active?

No, I implemented two versions of the API but neither got merged yet.

[mattheworiordan]

Ok, can we merge?

[paddybyers]

This is merged now in 0.8.1

[mattheworiordan]

Thanks, is 0.8.1 deployed to the CDN & NPM?

[paddybyers]

CDN yes, NPM no.

[mattheworiordan]

Ok, published to npm now as well