Modified public interface, instead of getRoutes/trunks use listTrunks… (

Azure#24918) ### Packages impacted by this PR Communication Phone-Numbers ### Issues associated with this PR https://skype.visualstudio.com/SPOOL/_workitems/edit/3149585/ ### Describe the problem that is addressed by this PR On the latest Internal Board Review, we got the feedback to change naming of functions retrieving the trunks and routes from "get" to "list". This PR aims to solve this. We don't provide options to set page size or skip, because: 1) The collections returned by ListTrunks and ListRoutes are short (usually 1~5 items), bigger collections don't make sense from use case perspective. 2) Underlying API doesnt support paging, therefore no cost would be saved with setting the page size. ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [ ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --------- Co-authored-by: Petr Švihlík <[email protected]>
azure-sdk · Feb 24, 2023 · 29afe14 · 29afe14
1 parent 054b79a
commit 29afe14
Show file tree

Hide file tree

Showing 13 changed files with 203 additions and 66 deletions.
diff --git a/sdk/communication/communication-phone-numbers/CHANGELOG.md b/sdk/communication/communication-phone-numbers/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### Features Added
 
 ### Breaking Changes
+- Changed public methods `getTrunks` to `listTrunks` and `getRoutes` to `listRoutes`.
 
 ### Bugs Fixed
 

diff --git a/...unication/communication-phone-numbers/review/communication-phone-numbers.api.md b/...unication/communication-phone-numbers/review/communication-phone-numbers.api.md
@@ -60,6 +60,14 @@ export interface ListOfferingsOptions extends OperationOptions {
 export interface ListPurchasedPhoneNumbersOptions extends OperationOptions {
 }
 
+// @public
+export interface ListSipRoutesOptions extends OperationOptions {
+}
+
+// @public
+export interface ListSipTrunksOptions extends OperationOptions {
+}
+
 // @public
 export interface ListTollFreeAreaCodesOptions extends Omit<PhoneNumbersListAreaCodesOptionalParams, "assignmentType" | "locality" | "administrativeDivision"> {
 }
@@ -207,9 +215,9 @@ export class SipRoutingClient {
     constructor(endpoint: string, credential: KeyCredential, options?: SipRoutingClientOptions);
     constructor(endpoint: string, credential: TokenCredential, options?: SipRoutingClientOptions);
     deleteTrunk(fqdn: string, options?: OperationOptions): Promise<void>;
-    getRoutes(options?: OperationOptions): Promise<SipTrunkRoute[]>;
     getTrunk(fqdn: string, options?: OperationOptions): Promise<SipTrunk>;
-    getTrunks(options?: OperationOptions): Promise<SipTrunk[]>;
+    listRoutes(options?: ListSipRoutesOptions): PagedAsyncIterableIterator<SipTrunkRoute>;
+    listTrunks(options?: ListSipTrunksOptions): PagedAsyncIterableIterator<SipTrunk>;
     setRoutes(routes: SipTrunkRoute[], options?: OperationOptions): Promise<SipTrunkRoute[]>;
     setTrunk(trunk: SipTrunk, options?: OperationOptions): Promise<SipTrunk>;
     setTrunks(trunks: SipTrunk[], options?: OperationOptions): Promise<SipTrunk[]>;

diff --git a/...nication/communication-phone-numbers/samples-dev/siprouting/getSipRoutingConfiguration.ts b/...nication/communication-phone-numbers/samples-dev/siprouting/getSipRoutingConfiguration.ts
@@ -17,14 +17,14 @@ export async function main() {
   const client = new SipRoutingClient(connectionString);
 
   // Get trunks
-  const trunks = await client.getTrunks();
-  for (const trunk of trunks) {
+  const trunks = await client.listTrunks();
+  for await (const trunk of trunks) {
     console.log(`Trunk ${trunk.fqdn}:${trunk.sipSignalingPort}`);
   }
 
   // Get routes
-  const routes = await client.getRoutes();
-  for (const route of routes) {
+  const routes = await client.listRoutes();
+  for await (const route of routes) {
     console.log(`Route ${route.name} with pattern ${route.numberPattern}`);
     console.log(`Route's trunks: ${route.trunks?.join()}`);
   }

diff --git a/...ation/communication-phone-numbers/samples-dev/siprouting/manageSipRoutingConfiguration.ts b/...ation/communication-phone-numbers/samples-dev/siprouting/manageSipRoutingConfiguration.ts
@@ -64,14 +64,14 @@ export async function main() {
   });
 
   // Get trunks
-  const trunks = await client.getTrunks();
-  for (const trunk of trunks) {
+  const trunks = await client.listTrunks();
+  for await (const trunk of trunks) {
     console.log(`Trunk ${trunk.fqdn}:${trunk.sipSignalingPort}`);
   }
 
   // Get routes
-  const routes = await client.getRoutes();
-  for (const route of routes) {
+  const routes = await client.listRoutes();
+  for await (const route of routes) {
     console.log(`Route ${route.name} with pattern ${route.numberPattern}`);
     console.log(`Route's trunks: ${route.trunks?.join()}`);
   }

diff --git a/sdk/communication/communication-phone-numbers/src/models.ts b/sdk/communication/communication-phone-numbers/src/models.ts
@@ -66,6 +66,16 @@ export interface ListLocalitiesOptions extends OperationOptions {
   administrativeDivision?: string;
 }
 
+/**
+ * Additional options that can be passed to list SIP routes.
+ */
+export interface ListSipRoutesOptions extends OperationOptions {}
+
+/**
+ * Additional options that can be passed to list SIP trunks.
+ */
+export interface ListSipTrunksOptions extends OperationOptions {}
+
 /**
  * Additional options that can be passed to the available offerings request.
  */

diff --git a/sdk/communication/communication-phone-numbers/src/sipRoutingClient.ts b/sdk/communication/communication-phone-numbers/src/sipRoutingClient.ts
@@ -11,10 +11,11 @@ import { InternalPipelineOptions } from "@azure/core-rest-pipeline";
 import { logger } from "./utils";
 import { SipRoutingClient as SipRoutingGeneratedClient } from "./generated/src/siprouting/sipRoutingClient";
 import { SipConfigurationPatch, SipRoutingError } from "./generated/src/siprouting/models";
-import { SipTrunk, SipTrunkRoute } from "./models";
+import { ListSipRoutesOptions, ListSipTrunksOptions, SipTrunk, SipTrunkRoute } from "./models";
 import { transformFromRestModel, transformIntoRestModel } from "./mappers";
 import { CommonClientOptions, OperationOptions } from "@azure/core-client";
 import { tracingClient } from "./generated/src/tracing";
+import { PagedAsyncIterableIterator } from "@azure/core-paging";
 
 export * from "./models";
 
@@ -99,14 +100,37 @@ export class SipRoutingClient {
   }
 
   /**
-   * Gets the SIP trunks.
+   * Lists the SIP trunks.
    * @param options - The options parameters.
    */
-  public async getTrunks(options: OperationOptions = {}): Promise<SipTrunk[]> {
-    return tracingClient.withSpan("SipRoutingClient-getTrunks", options, async (updatedOptions) => {
-      const config = await this.client.getSipConfiguration(updatedOptions);
-      return transformFromRestModel(config.trunks);
-    });
+  public listTrunks(options: ListSipTrunksOptions = {}): PagedAsyncIterableIterator<SipTrunk> {
+    const { span, updatedOptions } = tracingClient.startSpan(
+      "SipRoutingClient-listTrunks",
+      options
+    );
+
+    try {
+      const iter = this.listTrunksPagingAll({ ...updatedOptions });
+      return {
+        next() {
+          return iter.next();
+        },
+        [Symbol.asyncIterator]() {
+          return this;
+        },
+        byPage: () => {
+          return this.listTrunksPagingPage({ ...updatedOptions });
+        },
+      };
+    } catch (e: any) {
+      span.setStatus({
+        status: "error",
+        error: e,
+      });
+      throw e;
+    } finally {
+      span.end();
+    }
   }
 
   /**
@@ -116,25 +140,47 @@ export class SipRoutingClient {
    */
   public async getTrunk(fqdn: string, options: OperationOptions = {}): Promise<SipTrunk> {
     return tracingClient.withSpan("SipRoutingClient-getTrunk", options, async (updatedOptions) => {
-      const trunks = await this.getTrunks(updatedOptions);
+      const trunks = await this.getTrunksInternal(updatedOptions);
       const trunk = trunks.find((value: SipTrunk) => value.fqdn === fqdn);
       if (trunk) {
         return trunk;
       }
-
       throw { code: "NotFound", message: "Not Found" } as SipRoutingError;
     });
   }
 
   /**
-   * Gets the SIP trunk routes.
+   * Lists the SIP trunk routes.
    * @param options - The options parameters.
    */
-  public async getRoutes(options: OperationOptions = {}): Promise<SipTrunkRoute[]> {
-    return tracingClient.withSpan("SipRoutingClient-getRoutes", options, async (updatedOptions) => {
-      const config = await this.client.getSipConfiguration(updatedOptions);
-      return config.routes || [];
-    });
+  public listRoutes(options: ListSipRoutesOptions = {}): PagedAsyncIterableIterator<SipTrunkRoute> {
+    const { span, updatedOptions } = tracingClient.startSpan(
+      "SipRoutingClient-listRoutes",
+      options
+    );
+
+    try {
+      const iter = this.listRoutesPagingAll({ ...updatedOptions });
+      return {
+        next() {
+          return iter.next();
+        },
+        [Symbol.asyncIterator]() {
+          return this;
+        },
+        byPage: () => {
+          return this.listRoutesPagingPage({ ...updatedOptions });
+        },
+      };
+    } catch (e: any) {
+      span.setStatus({
+        status: "error",
+        error: e,
+      });
+      throw e;
+    } finally {
+      span.end();
+    }
   }
 
   /**
@@ -212,7 +258,7 @@ export class SipRoutingClient {
         ...patch,
       };
       const config = await this.client.patchSipConfiguration(payload);
-      const storedRoutes = config.routes || (await this.getRoutes(updatedOptions));
+      const storedRoutes = config.routes || (await this.getRoutesInternal(updatedOptions));
       return storedRoutes;
     });
   }
@@ -241,4 +287,44 @@ export class SipRoutingClient {
       }
     );
   }
+
+  private async getRoutesInternal(options: OperationOptions): Promise<SipTrunkRoute[]> {
+    const config = await this.client.getSipConfiguration(options);
+    return config.routes || [];
+  }
+
+  private async getTrunksInternal(options: OperationOptions): Promise<SipTrunk[]> {
+    const config = await this.client.getSipConfiguration(options);
+    return transformFromRestModel(config.trunks);
+  }
+
+  private async *listRoutesPagingAll(
+    options?: ListSipRoutesOptions
+  ): AsyncIterableIterator<SipTrunkRoute> {
+    for await (const page of this.listRoutesPagingPage(options)) {
+      yield* page;
+    }
+  }
+
+  private async *listTrunksPagingAll(
+    options?: ListSipTrunksOptions
+  ): AsyncIterableIterator<SipTrunk> {
+    for await (const page of this.listTrunksPagingPage(options)) {
+      yield* page;
+    }
+  }
+
+  private async *listTrunksPagingPage(
+    options: ListSipTrunksOptions = {}
+  ): AsyncIterableIterator<SipTrunk[]> {
+    const apiResult = await this.getTrunksInternal(options as OperationOptions);
+    yield apiResult;
+  }
+
+  private async *listRoutesPagingPage(
+    options: ListSipRoutesOptions = {}
+  ): AsyncIterableIterator<SipTrunkRoute[]> {
+    const apiResult = await this.getRoutesInternal(options as OperationOptions);
+    yield apiResult;
+  }
 }
diff --git a/sdk/communication/communication-phone-numbers/test/internal/siprouting/headers.spec.ts b/sdk/communication/communication-phone-numbers/test/internal/siprouting/headers.spec.ts
@@ -27,7 +27,8 @@ describe("SipRoutingClient - headers", function () {
 
   it("calls the spy", async function () {
     const spy = sinon.spy(getTrunksHttpClient, "sendRequest");
-    await client.getTrunks();
+    const iter = await client.listTrunks();
+    await iter.next();
     sinon.assert.calledOnce(spy);
 
     request = spy.getCall(0).args[0];
@@ -67,7 +68,8 @@ describe("SipRoutingClient - headers", function () {
     });
 
     const spy = sinon.spy(getTrunksHttpClient, "sendRequest");
-    await client.getTrunks();
+    const iter = await client.listTrunks();
+    await iter.next();
     sinon.assert.calledOnce(spy);
 
     request = spy.getCall(0).args[0];
@@ -86,7 +88,8 @@ describe("SipRoutingClient - headers", function () {
     });
 
     const spy = sinon.spy(getTrunksHttpClient, "sendRequest");
-    await client.getTrunks();
+    const iter = await client.listTrunks();
+    await iter.next();
     sinon.assert.calledOnce(spy);
 
     request = spy.getCall(0).args[0];
@@ -103,7 +106,8 @@ describe("SipRoutingClient - headers", function () {
     });
 
     const spy = sinon.spy(getTrunksHttpClient, "sendRequest");
-    await client.getTrunks();
+    const iter = await client.listTrunks();
+    await iter.next();
     sinon.assert.calledOnce(spy);
 
     request = spy.getCall(0).args[0];

diff --git a/sdk/communication/communication-phone-numbers/test/public/siprouting/deleteTrunk.spec.ts b/sdk/communication/communication-phone-numbers/test/public/siprouting/deleteTrunk.spec.ts
@@ -6,13 +6,14 @@ import { Context } from "mocha";
 
 import { SipRoutingClient } from "../../../src";
 
-import { isPlaybackMode, Recorder } from "@azure-tools/test-recorder";
+import { Recorder, isPlaybackMode } from "@azure-tools/test-recorder";
 import { SipTrunk } from "../../../src/models";
 import {
   clearSipConfiguration,
   createRecordedClient,
   createRecordedClientWithToken,
   getUniqueFqdn,
+  listAllTrunks,
   resetUniqueFqdns,
 } from "./utils/recordedClient";
 import { matrix } from "@azure/test-utils";
@@ -50,19 +51,19 @@ matrix([[true, false]], async function (useAad) {
       };
       const storedTrunk = await client.setTrunk(trunk);
       assert.deepEqual(storedTrunk, trunk);
-      assert.exists((await client.getTrunks()).find((value) => value.fqdn === trunk.fqdn));
+      assert.exists((await listAllTrunks(client)).find((value) => value.fqdn === trunk.fqdn));
 
       await client.deleteTrunk(testFqdn);
 
-      assert.notExists((await client.getTrunks()).find((value) => value.fqdn === trunk.fqdn));
+      assert.notExists((await listAllTrunks(client)).find((value) => value.fqdn === trunk.fqdn));
     });
 
     it("cannot delete a not existing trunk but succeeds", async () => {
       await client.setTrunks([]);
 
       await client.deleteTrunk("notExisting.fqdn.com");
 
-      const storedTrunks = await client.getTrunks();
+      const storedTrunks = await listAllTrunks(client);
       assert.isNotNull(storedTrunks);
       assert.isArray(storedTrunks);
       assert.isEmpty(storedTrunks);

diff --git a/sdk/communication/communication-phone-numbers/test/public/siprouting/getRoutes.spec.ts b/sdk/communication/communication-phone-numbers/test/public/siprouting/getRoutes.spec.ts
@@ -7,11 +7,12 @@ import { Context } from "mocha";
 import { SipRoutingClient } from "../../../src";
 
 import { matrix } from "@azure/test-utils";
-import { isPlaybackMode, Recorder } from "@azure-tools/test-recorder";
+import { Recorder, isPlaybackMode } from "@azure-tools/test-recorder";
 import {
   clearSipConfiguration,
   createRecordedClient,
   createRecordedClientWithToken,
+  listAllRoutes,
 } from "./utils/recordedClient";
 
 matrix([[true, false]], async function (useAad) {
@@ -38,13 +39,13 @@ matrix([[true, false]], async function (useAad) {
     });
 
     it("can retrieve routes", async () => {
-      assert.isArray(await client.getRoutes());
+      assert.isArray(await listAllRoutes(client));
     });
 
     it("can retrieve empty routes", async () => {
       await client.setRoutes([]);
 
-      const routes = await client.getRoutes();
+      const routes = await listAllRoutes(client);
 
       assert.isNotNull(routes);
       assert.isArray(routes);
@@ -68,7 +69,7 @@ matrix([[true, false]], async function (useAad) {
       ];
       await client.setRoutes(expectedRoutes);
 
-      const routes = await client.getRoutes();
+      const routes = await listAllRoutes(client);
 
       assert.isNotNull(routes);
       assert.isArray(routes);