Formatting returned paging URLs

[annatisch]

Hi,

As raised recently in issue #1217, currently the Python generator assumes that any "nextLink" URL returned by a service will be a complete URL.
Obviously this is not the case and all (please correct me if I'm wrong) of the following may apply:

• The server returns only a partial URL, that must be concatenated to the client's base URL
• The server returns a URL that must be additionally formatted with client parameters (e.g. the RBAC client requires the nextLink URLs be formatted with the tenant ID)
• The server returns a URL that must be formatted with additional query parameters (e.g. api-verion) before it will become valid (even if the URL already contains query parameters).

Currently none of the above scenarios are handled by Python - are we able to confirm which are/are not covered by other languages?

I started making a fix for the Python generator to handle some of these scenarios, though @xingwu1 has suggested that this would be better handled by extending the x-ms-pagable extension to include a parameter marking whether the returned URL should be considered complete or whether it requires formatting (concatenating, path parameters, query parameters) in order to be valid.

So - looking for thoughts/feedback on the current status and the best way to approach this.

If all the other languages already handle these cases, then I will continue to work on a solution specific to the Python generator.

Cheers.

[lmazuel]

As an example, the nextLink of GraphRbac is:

directoryObjects/$/Microsoft.DirectoryServices.User?$skipToken=X’445..5'

while the valid Url to get the page is:

https://graph.windows.net/myad.onmicrosoft.com/directoryObjects/$/Microsoft.DirectoryServices.User?$skipToken=X’445..5'&api-version=1.6

[tbombach]

I wrote a long breakdown of this issue, and re-stated the context to get anyone up to speed if they missed the discussion last time. I put some options at the end and recommended one, so let's talk about this during the discussion meeting tomorrow.

Problem summary

The Graph.RBAC spec has operations that return a pageable response (i.e. with value and odata.nextLink properties). The guidance for Azure REST APIs is for nextLink to be a complete URL, but Graph.RBAC only returns a fragment: directoryObjects/$/Microsoft.DirectoryServices.User?$skipToken=X’445343..500000'. The $skipToken query parameter lets the service know which page to return. AutoRest generated paging code is not currently written to handle anything other than a full URL.

Graph.RBAC service expects that a user would take the nextLink response value, append it to their base url and tenant id, then append the api-version query parameter (which is required).

Example (thanks Laurent!):

1. User sends request to https://graph.windows.net/myad.onmicrosoft.com/groups?api-version=1.6
2. User receives a response with an odata.nextLink value of directoryObjects/$/Microsoft.DirectoryServices.User?$skipToken=X’445343..500000'
3. Users can get the next page by sending a request to: https://graph.windows.net/myad.onmicrosoft.com/directoryObjects/$/Microsoft.DirectoryServices.User?$skipToken=X’445343..500000'&api-version=1.6

Graph.RBAC Swagger spec

Each pageable operation in the spec (e.g. groups) defines an operationName in x-ms-pageable. This means that the "Next" method (to get the next page) will be defined by that operation (which are located in x-ms-paths).

The corresponding ListNext operation in x-ms-paths can be called with the nextLink argument to get the next page of results. This x-ms-paths operation is of the form "/{tenantID}/{nextLink}" (i.e. a path that consists of a two parameters called tenantID and nextLink).

Current behavior

The behavior depends on if operationName is used in x-ms-pageable. Since the Graph.RBAC spec uses operationName to refer to a x-ms-paths path, I will describe that logic.

C# SDK Code

Handles paging extension by:

1. Using the operation referred to by operationName in the x-ms-pageable extension
2. Using the same logic as a normal method, which results in:
3. Starting with the base URL and adding the path from x-ms-paths
4. Replacing {nextLink} with the nextLink parameter from the method argument
5. Replacing {tenantID} with tenantID from the service client
6. Setting the query parameters (such as apiVersion) by appending '?' to the end of the url, then parameter values, joined by '&'
   • This currently fails in C#. Since the nextLink already has a query parameter ($skiptoken), the path ends up looking like: https://graph.windows.net/myad.onmicrosoft.com/directoryObjects/$/Microsoft.DirectoryServices.User?$skipToken=X’445..5'?api-version=1.6

Python (possibly others too) Graph.RBAC SDK Code

Handles paging extension by:

1. If a method is a paging method, uses a different template from a normal method
2. In the AzurePagingMethodTemplate.cshtml template, writes a method that takes a nextLink argument

• If it is null, constructs the URL as usual (replace path parameters, add query parameters, etc.)
• If the nextLink argument is not null, uses it as the URL
  • This currently fail in Python SDK. Since the nextLink is a fragment, it cannot be used as the nextLink. It requires the base path, tenantID path parameter, and api-version query parameter.

Workarounds

C# SDK Code

The issue can be worked around in .NET by changing the logic in the generated code to append query parameters to the built URL, not set them (i.e. by starting with '&' instead of '?'): See this commit that manually changed the logic in .NET SDK

Options

1) Mandate that nextLink must be a complete URL

Cons: this prevents us from supporting Graph.RBAC for the foreseeable future.

2) Add a configurable/overridable way to construct the URL in each language

Cons: More functionality to support (for all language teams), wouldn't be standard across all languages, wouldn't likely be used by anyone else (since pageable is an Azure extension, and they all should be following the guidelines), would duplicate some AutoRest logic, since it would be doing the parameter replacements that generated code already does.

3) (Recommended) Standardize the paging logic for all languages so that fragment URLs can be supported by using operationName

Cons: Work for languages that don't currently follow this logic, needs to be well-documented

The logic for handling x-ms-pageable would be:

1. If operationName is used, skip creating a Next() page method
2. The operation specified in operationName should follow the normal method logic
3. Start with the base url
4. Concat the operation path (e.g. {tenantID}/{nextLink} in the case of Graph.RBAC)
5. Perform path replacements
   • This includes replacing {nextLink} with the nextLink method argument
6. Append any query parameters defined for the Next() operation.
   • This needs to take into account that query parameters might already exist in the url, since nextLink could have query parameters.

I don't think the logic for x-ms-pageable should change if operationName is omitted. We still will create a method that expects the nextLink argument to be a complete URL.

[annatisch]

Thanks @tbombach for this write up!

So just to clarify - your recommendation is that the fix for this scenario will happen purely in the individual language generators (as opposed to within the core/extension projects) and will based on the presence of the operationName field?

Cheers.

[tbombach]

@annatisch Correct, it would be in each generator, no changes to core or extensions. I tried to think of an addition to the extension that would make it easier for each generator, but making it flexible enough for the RBAC scenario requires information that would just duplicate info that already exists in the Next operation definition (e.g. path & query parameters).

I'll send out a PR that adds the specific Graph.RBAC scenario to the test server (and updates the C# generator to handle it). I wanted to give everyone a chance to comment on the proposal in today's meeting before we committed to this plan though.

[annatisch]

@tbombach - thanks! I have finished these changes for the Python generator.
Though it would be good to have a generator test for it - I can build an example from the graphRBAC spec and add it to the tests if you like?

[tbombach]

@annatisch Awesome! I got started on a generator test (it's in this branch: https://github.com/tbombach/autorest/tree/paging-fragment-test). I'm not sure if I'll be able to get it checked in before early next week, so feel free to use it as a starting spot if you want to finish it before then. Otherwise I'm happy to finish it and get it checked in.

[annatisch]

@tbombach - can you confirm which, if any, languages still need to implement support for this so we can label accordingly?
To my knowledge it's only C# and Python - but perhaps you got some of the others working too?

[lmazuel]

@tbombach @annatisch Python release of GraphRbac with this fix

[tbombach]

I'm closing this issue in favor of #1426, which summarizes the issue and specifically tracks the changes that have to be made for the generators for the other languages. Hopefully that makes it easier for whoever will make the changes.