Improve generated paging methods when next operation is not specified

[xirzec]

When a swagger has x-ms-pageable info for an operation, it may only have nextLinkName and itemName specified, but not operationName. In these cases, we automatically generate a 'next' operation for fetching subsequent pages of results.

However today we solve this by cloning the original operation, which tends to drag in query parameters that are not required on subsequent pages because they are embedded inside the nextLink itself. Other langauges seem to use the nextLink as-is to avoid this trouble, but we write an OperationSpec that treats the next link as a path parameter and recalculate the final url by applying any specified query parameters.

This change modifies our code generator to elide any query parameters when constructing the next page operation, which should avoid cases where consumers mistakenly double-pass parameters or when services uses query parameters to encode pagination via top and skip.

Fixes #1347

This PR also incorporates some minor reference doc feedback for the getContinuationToken helper from #1199

[qiaozha]

Hi @xirzec, Not sure if this issue could help Azure/autorest#4454. Previously, when we support metrics advisor, They want the client side to use POST to get the next page data. we kind of use m4 directive to add operationName to support their scenario. we had a discussion about nextLink and operationName in x-ms-pageable.

[deyaaeldeen]

nitpick: Could the passed lambda be simplified as follows?

Suggested change

	nextLinkMethod.parameters = nextLinkMethod.parameters.filter(param => {
	return param.protocol.http?.in !== ParameterLocation.Query;
	});
	nextLinkMethod.parameters = nextLinkMethod.parameters.filter(param => param.protocol.http?.in !== ParameterLocation.Query);

[deyaaeldeen]

Also, could this change be a breaking change? are there query params out there that are expected to be added to the nextLink URL for some reason? I am not saying this is an ideal behavior, I just wonder if we're breaking any one.

[xirzec]

While I'd never say never because we've seen some pretty wild service behaviors, at least according to our API guidelines this shouldn't be a problem: https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#981-server-driven-paging

Clients MUST treat the continuation URL as opaque, which means that query options may not be changed while iterating over a set of partial results.

[deyaaeldeen]

Looks good to me and left one comment about it being potential breaking behavior.

[xirzec]

Hi @xirzec, Not sure if this issue could help Azure/autorest#4454. Previously, when we support metrics advisor, They want the client side to use POST to get the next page data. we kind of use m4 directive to add operationName to support their scenario. we had a discussion about nextLink and operationName in x-ms-pageable.

Is the idea that we could improve our automatic creation of the 'next' operation to allow for POST without requiring the swagger author to explicitly create the next operation and reference it using operationName? It seems reasonable enough that if the x-ms-pageable operation itself is a POST we should have the next also be a POST...

[xirzec]

ah I see we override it here:

autorest.typescript/packages/autorest.typescript/src/transforms/extensions.ts

Line 166 in cd004be

nextLinkRequestProtocol.method = "GET";

I wonder if we'd break anyone by changing it...

[qiaozha]

Is the idea that we could improve our automatic creation of the 'next' operation to allow for POST without requiring the swagger author to explicitly create the next operation and reference it using operationName? It seems reasonable enough that if the x-ms-pageable operation itself is a POST we should have the next also be a POST...

I remember in that previous discussion, Renhe asked autorest team to add a httpVerb in x-ms-pageble to indicate what kind of HTTP method we should use when calling next link, but they suggest us to use operationName to specify POST. and nextLink would become a parameter of operationName operation. From this PR's desciption, it feels like we are doing exactly the same thing ?

[qiaozha]

Looks like the CI is passing.

[xirzec]

Is the idea that we could improve our automatic creation of the 'next' operation to allow for POST without requiring the swagger author to explicitly create the next operation and reference it using operationName? It seems reasonable enough that if the x-ms-pageable operation itself is a POST we should have the next also be a POST...

I remember in that previous discussion, Renhe asked autorest team to add a httpVerb in x-ms-pageble to indicate what kind of HTTP method we should use when calling next link, but they suggest us to use operationName to specify POST. and nextLink would become a parameter of operationName operation. From this PR's desciption, it feels like we are doing exactly the same thing ?

I'm fine with adding a new swagger attribute for this, though we'd have to get it into autorest core first.

This PR isn't actually adding new support; it's tweaking our existing logic for what happens when someone specifies a nextLink but not an operationName.

Today we clone the base operation, force the method to be a GET, and insert a new path parameter that essentially replaces the entire URL. This PR does the extra step of removing any existing query parameters from the original operation so we don't duplicate or overwrite query parameters in the nextLink itself after it replaces the existing baseUrl.