Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve generated paging methods when next operation is not specified #1661

Merged
merged 6 commits into from
Nov 29, 2022
Merged

Improve generated paging methods when next operation is not specified #1661

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08bca19
Tweak comment based on feedback
xirzec Nov 11, 2022
bb39478
filter out query params when generating nextlink operations
xirzec Nov 11, 2022
1ee035f
update integration packages
xirzec Nov 11, 2022
47d3b54
update smoke
xirzec Nov 11, 2022
0ed28e7
Merge branch 'main' into pagingFeedback
xirzec Nov 28, 2022
5397f4d
PR feedback
xirzec Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
8 changes: 4 additions & 4 deletions packages/autorest.typescript/src/pagingHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ export interface PageInfo {
const pageMap = new WeakMap<object, PageInfo>();

/**
* Given a result page from a pageable operation, returns a
* continuation token that can be used to begin paging from
* Given the last `.value` produced by the `byPage` iterator,
* returns a continuation token that can be used to begin paging from
* that point later.
* @param page A result object from calling .byPage() on a paged operation.
* @returns The continuation token that can be passed into byPage().
* @param page An object from accessing `value` on the IteratorResult from a `byPage` iterator.
* @returns The continuation token that can be passed into byPage() during future calls.
*/
export function getContinuationToken(page: unknown): string | undefined {
if (typeof page !== "object" || page === null) {
Expand Down
15 changes: 14 additions & 1 deletion packages/autorest.typescript/src/transforms/extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,22 @@ function addPageableMethods(codeModel: CodeModel) {
}
);

// Filter out query parameters from the original operation.
if (nextLinkMethod.parameters) {
nextLinkMethod.parameters = nextLinkMethod.parameters.filter(param => {
return param.protocol.http?.in !== ParameterLocation.Query;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (nextLinkMethod.signatureParameters) {
nextLinkMethod.updateSignatureParameters();
}
}

// Ensure all overloads support the nextLink parameter.
for (const request of nextLinkMethod.requests ?? []) {
const parameters = request.parameters ?? [];
let parameters = request.parameters ?? [];
parameters = parameters.filter(param => {
return param.protocol.http?.in !== ParameterLocation.Query;
});
parameters.push(nextLinkParameter);
request.parameters = parameters;
}
Expand Down
21 changes: 0 additions & 21 deletions ...rest.typescript/test/integration/generated/appconfiguration/src/appConfigurationClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,6 @@ const getKeysNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [Parameters.name, Parameters.apiVersion, Parameters.after],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.accept,
Expand All @@ -926,13 +925,6 @@ const getKeyValuesNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [
Parameters.apiVersion,
Parameters.after,
Parameters.key,
Parameters.label,
Parameters.select
],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.syncToken,
Expand All @@ -953,12 +945,6 @@ const getLabelsNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [
Parameters.name,
Parameters.apiVersion,
Parameters.after,
Parameters.select4
],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.syncToken,
Expand All @@ -979,13 +965,6 @@ const getRevisionsNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [
Parameters.apiVersion,
Parameters.after,
Parameters.key,
Parameters.label,
Parameters.select5
],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.syncToken,
Expand Down
26 changes: 0 additions & 26 deletions packages/autorest.typescript/test/integration/generated/appconfiguration/src/models/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,10 +634,6 @@ export type CheckRevisionsResponse = AppConfigurationClientCheckRevisionsHeaders

/** Optional parameters. */
export interface GetKeysNextOptionalParams extends coreClient.OperationOptions {
/** A filter for the name of the returned keys. */
name?: string;
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
}
Expand All @@ -649,16 +645,8 @@ export type GetKeysNextResponse = AppConfigurationClientGetKeysNextHeaders &
/** Optional parameters. */
export interface GetKeyValuesNextOptionalParams
extends coreClient.OperationOptions {
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
/** A filter used to match keys. */
key?: string;
/** A filter used to match labels */
label?: string;
/** Used to select what fields are present in the returned resource(s). */
select?: Get6ItemsItem[];
}

/** Contains response data for the getKeyValuesNext operation. */
Expand All @@ -668,14 +656,8 @@ export type GetKeyValuesNextResponse = AppConfigurationClientGetKeyValuesNextHea
/** Optional parameters. */
export interface GetLabelsNextOptionalParams
extends coreClient.OperationOptions {
/** A filter for the name of the returned labels. */
name?: string;
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
/** Used to select what fields are present in the returned resource(s). */
select?: string[];
}

/** Contains response data for the getLabelsNext operation. */
Expand All @@ -685,16 +667,8 @@ export type GetLabelsNextResponse = AppConfigurationClientGetLabelsNextHeaders &
/** Optional parameters. */
export interface GetRevisionsNextOptionalParams
extends coreClient.OperationOptions {
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
/** A filter used to match keys. */
key?: string;
/** A filter used to match labels */
label?: string;
/** Used to select what fields are present in the returned resource(s). */
select?: Enum4[];
}

/** Contains response data for the getRevisionsNext operation. */
Expand Down
8 changes: 4 additions & 4 deletions packages/autorest.typescript/test/integration/generated/appconfiguration/src/pagingHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ export interface PageInfo {
const pageMap = new WeakMap<object, PageInfo>();

/**
* Given a result page from a pageable operation, returns a
* continuation token that can be used to begin paging from
* Given the last `.value` produced by the `byPage` iterator,
* returns a continuation token that can be used to begin paging from
* that point later.
* @param page A result object from calling .byPage() on a paged operation.
* @returns The continuation token that can be passed into byPage().
* @param page An object from accessing `value` on the IteratorResult from a `byPage` iterator.
* @returns The continuation token that can be passed into byPage() during future calls.
*/
export function getContinuationToken(page: unknown): string | undefined {
if (typeof page !== "object" || page === null) {
Expand Down
21 changes: 0 additions & 21 deletions ...ypescript/test/integration/generated/appconfigurationexport/src/appConfigurationClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,6 @@ const getKeysNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [Parameters.name, Parameters.apiVersion, Parameters.after],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.accept,
Expand All @@ -927,13 +926,6 @@ const getKeyValuesNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [
Parameters.apiVersion,
Parameters.after,
Parameters.key,
Parameters.label,
Parameters.select
],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.syncToken,
Expand All @@ -954,12 +946,6 @@ const getLabelsNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [
Parameters.name,
Parameters.apiVersion,
Parameters.after,
Parameters.select4
],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.syncToken,
Expand All @@ -980,13 +966,6 @@ const getRevisionsNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.ErrorModel
}
},
queryParameters: [
Parameters.apiVersion,
Parameters.after,
Parameters.key,
Parameters.label,
Parameters.select5
],
urlParameters: [Parameters.endpoint, Parameters.nextLink],
headerParameters: [
Parameters.syncToken,
Expand Down
26 changes: 0 additions & 26 deletions ...autorest.typescript/test/integration/generated/appconfigurationexport/src/models/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,10 +634,6 @@ export type CheckRevisionsResponse = AppConfigurationClientCheckRevisionsHeaders

/** Optional parameters. */
export interface GetKeysNextOptionalParams extends coreClient.OperationOptions {
/** A filter for the name of the returned keys. */
name?: string;
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
}
Expand All @@ -649,16 +645,8 @@ export type GetKeysNextResponse = AppConfigurationClientGetKeysNextHeaders &
/** Optional parameters. */
export interface GetKeyValuesNextOptionalParams
extends coreClient.OperationOptions {
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
/** A filter used to match keys. */
key?: string;
/** A filter used to match labels */
label?: string;
/** Used to select what fields are present in the returned resource(s). */
select?: Get6ItemsItem[];
}

/** Contains response data for the getKeyValuesNext operation. */
Expand All @@ -668,14 +656,8 @@ export type GetKeyValuesNextResponse = AppConfigurationClientGetKeyValuesNextHea
/** Optional parameters. */
export interface GetLabelsNextOptionalParams
extends coreClient.OperationOptions {
/** A filter for the name of the returned labels. */
name?: string;
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
/** Used to select what fields are present in the returned resource(s). */
select?: string[];
}

/** Contains response data for the getLabelsNext operation. */
Expand All @@ -685,16 +667,8 @@ export type GetLabelsNextResponse = AppConfigurationClientGetLabelsNextHeaders &
/** Optional parameters. */
export interface GetRevisionsNextOptionalParams
extends coreClient.OperationOptions {
/** Instructs the server to return elements that appear after the element referred to by the specified token. */
after?: string;
/** Requests the server to respond with the state of the resource at the specified time. */
acceptDatetime?: string;
/** A filter used to match keys. */
key?: string;
/** A filter used to match labels */
label?: string;
/** Used to select what fields are present in the returned resource(s). */
select?: Enum4[];
}

/** Contains response data for the getRevisionsNext operation. */
Expand Down
8 changes: 4 additions & 4 deletions ...autorest.typescript/test/integration/generated/appconfigurationexport/src/pagingHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ export interface PageInfo {
const pageMap = new WeakMap<object, PageInfo>();

/**
* Given a result page from a pageable operation, returns a
* continuation token that can be used to begin paging from
* Given the last `.value` produced by the `byPage` iterator,
* returns a continuation token that can be used to begin paging from
* that point later.
* @param page A result object from calling .byPage() on a paged operation.
* @returns The continuation token that can be passed into byPage().
* @param page An object from accessing `value` on the IteratorResult from a `byPage` iterator.
* @returns The continuation token that can be passed into byPage() during future calls.
*/
export function getContinuationToken(page: unknown): string | undefined {
if (typeof page !== "object" || page === null) {
Expand Down
8 changes: 4 additions & 4 deletions packages/autorest.typescript/test/integration/generated/customUrlPaging/src/pagingHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ export interface PageInfo {
const pageMap = new WeakMap<object, PageInfo>();

/**
* Given a result page from a pageable operation, returns a
* continuation token that can be used to begin paging from
* Given the last `.value` produced by the `byPage` iterator,
* returns a continuation token that can be used to begin paging from
* that point later.
* @param page A result object from calling .byPage() on a paged operation.
* @returns The continuation token that can be passed into byPage().
* @param page An object from accessing `value` on the IteratorResult from a `byPage` iterator.
* @returns The continuation token that can be passed into byPage() during future calls.
*/
export function getContinuationToken(page: unknown): string | undefined {
if (typeof page !== "object" || page === null) {
Expand Down
1 change: 0 additions & 1 deletion ....typescript/test/integration/generated/datafactory/src/operations/dataFlowDebugSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,6 @@ const queryByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...es/autorest.typescript/test/integration/generated/datafactory/src/operations/dataFlows.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...ges/autorest.typescript/test/integration/generated/datafactory/src/operations/datasets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
2 changes: 0 additions & 2 deletions ...es/autorest.typescript/test/integration/generated/datafactory/src/operations/factories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,6 @@ const listNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand All @@ -593,7 +592,6 @@ const listByResourceGroupNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...t.typescript/test/integration/generated/datafactory/src/operations/integrationRuntimes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,7 +1096,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...torest.typescript/test/integration/generated/datafactory/src/operations/linkedServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...pescript/test/integration/generated/datafactory/src/operations/managedPrivateEndpoints.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...ypescript/test/integration/generated/datafactory/src/operations/managedVirtualNetworks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
1 change: 0 additions & 1 deletion ...s/autorest.typescript/test/integration/generated/datafactory/src/operations/operations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ const listNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [Parameters.$host, Parameters.nextLink],
headerParameters: [Parameters.accept],
serializer
Expand Down
1 change: 0 additions & 1 deletion ...es/autorest.typescript/test/integration/generated/datafactory/src/operations/pipelines.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,6 @@ const listByFactoryNextOperationSpec: coreClient.OperationSpec = {
bodyMapper: Mappers.CloudError
}
},
queryParameters: [Parameters.apiVersion],
urlParameters: [
Parameters.$host,
Parameters.nextLink,
Expand Down
Loading