Paging support: byPage not exposing settings parameter

[danielgerlag]

When generating an operation with paging, it mostly works well, except the byPage method that is generated does not pass the settings parameter down as defined on the PagedAsyncIterableIterator interface.

export interface PagedAsyncIterableIterator<T, PageT = T[], PageSettingsT = PageSettings> {
  next(): Promise<IteratorResult<T, T>>;
  [Symbol.asyncIterator](): PagedAsyncIterableIterator<T, PageT, PageSettingsT>;
  byPage: (settings?: PageSettingsT) => AsyncIterableIterator<PageT>;
}

Generate code:

public listAvailableWorkers(
    options?: RouterGetAvailableWorkersOptionalParams
  ): PagedAsyncIterableIterator<Worker> {
    const iter = this.getAvailableWorkersPagingAll(options);
    return {
      next() {
        return iter.next();
      },
      [Symbol.asyncIterator]() {
        return this;
      },
      byPage: () => {
        return this.getAvailableWorkersPagingPage(options);
      }
    };
  }

[sarangan12]

This looks like a valid bug. We need to pass the settings in the generated byPage method.

[sarangan12]

I had a teams meeting with @deyaaeldeen. Here is the gist of the conversation:

1. Today, we have a paging public API implementation such as:

public list(options?: VirtualMachineImageTemplatesListOptionalParams): PagedAsyncIterableIterator<ImageTemplate> {
    const iter = this.listPagingAll(options);
    return {
      next() {
        return iter.next();
      },
      [Symbol.asyncIterator]() {
        return this;
      },
      byPage: () => {
        return this.listPagingPage(options);
      }
    };
}

2. In the above code, the byPage method does not take any parameters. But, according to the definition of PagedAsyncIterableIterator defined in the core-paging package
3. So, the byPage method implemented in step 1, should be modified as:

........
byPage: (settings?: PageSettings) => {
......

4. PageSettings (used in Step 3) is defined in the core-paging package as:

/**
 * An interface that tracks the settings for paged iteration
 */
export interface PageSettings {
  /**
   * The token that keeps track of where to continue the iterator
   */
  continuationToken?: string;
  /**
   * The size of the page during paged iteration
   */
  maxPageSize?: number;
}

5. The continuationToken consists of the URL that should be used for polling. The maxPageSize should be used as value of top during the first call only. After that, You can ignore the value.

During the discussion, Deyaa Pointed that there is an abstraction created in the core-paging package. This abstraction could be used to refine this task. Deyaa has implemented similar changes in ai-text-analytics This can be used for reference.

@deyaaeldeen Am I clear in formulating our discussion? Is there anything I missed?

[deyaaeldeen]

Yes! thanks for summarizing!

one nit:

The continuationToken consists of the URL that should be used for polling

for getting the next page, typically named nextLink.

[MRayermannMSFT]

@danielgerlag @deyaaeldeen any updates on this?

[deyaaeldeen]

@sarangan12, I don't have an update for the customers on this issue, could you please provide one?

[xirzec]

Exposing settings and treating continuationToken as the nextLink is easy enough, but something I'm now struggling with is how we intend for consumers to retrieve the nextLink on the results. I am wary of doing any kind of non-enumerable property tricks, so it seems like we need something like a wrapper type around the existing page results.

Unfortunately, since byPage() already returns AsyncIterableIterator<TPage> and we don't want to make a breaking change, I wonder if the only solution is to add a new method like:

export interface PagedResult<TPage> {
   page: TPage;
   continuationToken?: string;
}
export interface PagedAsyncIterableIterator<TElement, TPage = TElement[], TPageSettings = PageSettings> {
    [Symbol.asyncIterator](): PagedAsyncIterableIterator<TElement, TPage, TPageSettings>;
    byPage: (settings?: TPageSettings) => AsyncIterableIterator<TPage>;
    byPagedResult: (settings?: TPageSettings) => AsyncIterableIterator<PagedResult<TPage>>;
    next(): Promise<IteratorResult<TElement>>;
}

The only alternative I can think of is maybe having an abstraction where the generated client annotates this information in a WeakMap and the consumer uses a helper method to check if the result page has an associated continuationToken or not.

@deyaaeldeen @MRayermannMSFT any thoughts on how you'd like this to be implemented?

[xirzec]

I think what I'm asking is how we'd solve #1326 while addressing this item

[deyaaeldeen]

@xirzec I agree, it is not an ideal situation. Extending the PagedAsyncIterableIterator interface with an extra method with the right return type makes sense to me. The only downside I can think of to this is the potential confusion this may cause to customers who don't need access to the continuationToken which I assume is the majority of them. One alternative that I suggested earlier to @MRayermannMSFT is referring customers to using the onResponse callback to access the raw response and extract the nextLink manually from there. [Sample].

@MRayermannMSFT I am curious to hear your feedback on my proposed alternative above and whether is it sufficient for your use case.

[bwateratmsft]

Could a class be created that takes PagedAsyncIterableIterator as a constructor parameter, and that exposes the necessary information? That would "hide" it from customers who don't need it, avoiding the confusion.

[MRayermannMSFT]

@xirzec

I wonder if the only solution is to add a new method like:

Ya this could work. Maybe named firstPage and it just returns the page and continuationToken. Not sure if the term "paged result" is good (what is a paged result anyways?). Could it be added onto IteratorResult or do we worry that would confuse people into making them think they need to do something with token?

Could a class be created that takes PagedAsyncIterableIterator as a constructor parameter, and that exposes the necessary information?

I don't not-like this either.

[xirzec]

@bwateratmsft Unless the construct that took the iterator had some secret means of accessing associated page data, I'm not sure if I understand how that would work.

I'm leaning back towards somehow tying this information into the lifetime of TPage since I think it should always be an object of some sort (array or otherwise) that we could weakly map state against.

How would we feel about something like this:

import { FooClient, getContinuationToken } from "@azure/foo";

const client = new FooClient();
const iterator = client.listFoo().byPage();
const firstPage = await iterator.next();
const continuationToken = getContinuationToken(firstPage);
const laterIterator = client.listFoo().byPage({continuationToken});
//  laterIterator starts where iterator left off

[bwateratmsft]

@bwateratmsft Unless the construct that took the iterator had some secret means of accessing associated page data, I'm not sure if I understand how that would work.

Not saying it's necessarily a good idea, but TypeScript / JavaScript lets you do anything. Such a class could peek under the hood to get what it needed.

[MRayermannMSFT]

@bwateratmsft Unless the construct that took the iterator had some secret means of accessing associated page data, I'm not sure if I understand how that would work.

I'm leaning back towards somehow tying this information into the lifetime of TPage since I think it should always be an object of some sort (array or otherwise) that we could weakly map state against.

How would we feel about something like this:

import { FooClient, getContinuationToken } from "@azure/foo";

const client = new FooClient();
const iterator = client.listFoo().byPage();
const firstPage = await iterator.next();
const continuationToken = getContinuationToken(firstPage);
const laterIterator = client.listFoo().byPage({continuationToken});
//  laterIterator starts where iterator left off

I'm curious as to what getContinuationToken is actually doing. Lots of packages have many different listing calls. Would you need a function for each type of TPage? Overall the idea seems "fine". Every idea so far has been acceptable really.

[xirzec]

@MRayermannMSFT I have an initial draft implementation here: https://github.com/Azure/autorest.typescript/blob/e45434438d16edac5c63a7ddd9a4322e9a25e025/packages/autorest.typescript/src/pagingHelper.ts

It's the same function for all pages, so the input parameter is not typed to any one particular page shape.

[xirzec]

@MRayermannMSFT just merged this, let me know if you have any feedback or issues with using it.

[MRayermannMSFT]

@xirzec what's the best way for me to get my hands on the changes so I can provide you feedback? I'm not seeing any differences with the packages I'm using, so I'm guessing they probably all need to be regenerated?

[xirzec]

@MRayermannMSFT yeah, we should be able to regenerate with the latest dev version of @autorest/typescript. Which packages do you need to have regenerated? Could you perhaps file an issue in our main repo with the list? https://github.com/Azure/azure-sdk-for-js/issues

[MRayermannMSFT]

Hey @xirzec , for the getting of initial feedback, regenerating @azure/arm-resources would be a good place to start. I've got some existing tests around our usage of it that make sure our continuation token related hacks work well when listing resource groups.

[xirzec]

@qiaozha - could you help with regenerating this package?

[kazrael2119]

Hi @MRayermannMSFT ,
The below is new sdk package
azure-arm-resources-5.1.0.zip

We would like you help us verify whether the package works as your expected before release, you can also learn more by this pr: Azure/azure-sdk-for-js#23688

Thanks.

[MRayermannMSFT]

@xirzec @qiaozha @kazrael2119

First, let me show you the code I ended up with for context:

And here's my feedback:

• My existing code (with a bunch of hacky workarounds) works well with the shared bits. So nice to know releasing wouldn't break what I currently have.
• I don't love that getContinuationToken has the unknown typing on its parameter. I had 0 idea what to pass in there when I first started trying to validate the new package. If y'all were able to pivot to getContinuationToken taking nextPage, then it could have the typing of IteratorResult<any, any>. But from taking a peek at the impl of all of this, I have a feeling that won't be possible, as at the time of calling setContinuationToken y'all only have the value, you don't have the IteratorResult. So if the typing can't be improved, an improved JS Doc comment for getContinuationToken would be great. Currently it says "Given a result page from a pageable operation...". The phrase "result page" definitely made me think I should be passing in the return value of next().
• Ultimately, as to whether or not the new package works without any workarounds, the answer is no. Passing in a top while listing a not-first page is still broken, as originally reported here. To workaround this issue, I need to do a hacky workaround like:
  
  Which is similar to one of the hacky workarounds in my existing code.

[xirzec]

Hey @MRayermannMSFT thanks for trying this out!

I hear you on the second point of feedback and agreed that we could at the very least improve the comment since it's not really feasible to scope this down to a type given you could have many different pageables in an SDK each with different page types. I always struggle with how to speak to iterators since I feel like most JS devs don't really understand or think about the iteration protocol and simply for/of or for await/of their way around them.

Would calling it the last value produced by the byPage iterator be clearer? In pagination (and most iterators) we don't use the return value feature of the iterator protocol.

I rewrote your above example slightly in a way that I think shows what is going on a bit better. One of our struggles with pagination is that it's not obvious that the list operation doesn't return a promise, but rather an object that implements the async iterator protocol:

const iterator = client.resourceGroups.list(options).byPage({continuationToken: options?.continuationToken);
const iteratorResult = await iterator.next();
if (!iteratorResult.done) {
   const nextPage = iteratorResult.value;
   return { resourceGroups: nextPage, continuationToken: getContinuationToken(nextPage) };
} else {
   return { resourceGroups: [] };
}

To your other point (about top) - it sounds like we need to resolve #1347 to fully solve your needs. I took a glance at it, and I think we might be able to get away with updating the generator to remove all query parameters when doing a next operation of an x-ms-pageable and trust that nextLink contains all necessary information -- does that sound accurate for the API you are using?

[MRayermannMSFT]

I rewrote your above example slightly in a way that I think shows what is going on a bit better.
One of our struggles with pagination is that it's not obvious that the list operation doesn't return a promise...

Haha, I think when we migrated from the super old SDKs we just assumed methods named like list still returned promises. Thanks for pointing out where we can drop the some awaits.

Would calling it the last value produced by the byPage iterator be clearer? In pagination (and most iterators) we don't use the return value feature of the iterator protocol.

Yes I think that would be clearer. Even more clearer would be if the comment put "value" and "byPage" in back-tics (`), and maybe even with a leading dot on "value". So like:

/**
 * ...the last `.value` produced by the `byPage` iterator...
 */
function getContinuationToken() {}

[MRayermannMSFT]

To your other point (about top) - it sounds like we need to resolve #1347 to fully solve your needs. I took a glance at it, and I think we might be able to get away with updating the generator to remove all query parameters when doing a next operation of an x-ms-pageable and trust that nextLink contains all necessary information -- does that sound accurate for the API you are using?

For the API I am using....yes I think that sounds accurate. For all Azure APIs, no idea! 😅 Maybe another way to look at it is to not duplicate query params?

[xirzec]

For the API I am using....yes I think that sounds accurate. For all Azure APIs, no idea! 😅 Maybe another way to look at it is to not duplicate query params?

The problem is we don't know anything about nextLink from the swagger spec, all we get is something like this on the operation:

"x-ms-pageable": {
          "nextLinkName": "nextLink",
          "itemName": "phoneNumbers"
        },

And at runtime while it may sound very sane to not duplicate, we do have APIs that allow for specifying the same query parameter multiple times (perhaps treating them as an OR condition) with different services wanting these values either concatenated (with comma, semicolon, pipes, or something custom!) or literally repeated e.g. (foo=1&foo=2) -- so the poor logic in ServiceClient really can't ascertain what was intended just from looking at the operation spec and operation args.

[MRayermannMSFT]

Sure, if the "continuation token" is a next link, then I think saying "we will use that link exactly and not add anything to it" is reasonable. I was merely thinking about APIs where the "continuation token" is not a link. Which in that case it hopefully isn't called nextLink.

[xirzec]

Sure, if the "continuation token" is a next link, then I think saying "we will use that link exactly and not add anything to it" is reasonable. I was merely thinking about APIs where the "continuation token" is not a link. Which in that case it hopefully isn't called nextLink.

Ah, yeah the pagination contract tries to abstract this better, but since x-ms-pageable I believe only works today with next links, we can probably use that as the hint to avoid query params.

[kazrael2119]

released here:https://www.npmjs.com/package/@azure/arm-resources/v/5.1.0