[Communication] - phone-numbers - Core V2 migration #20633

danielortega-msft · 2022-03-03T00:44:19Z

Packages impacted by this PR

@azure/communication-phone-numbers

Issues associated with this PR

Describe the problem that is addressed by this PR

Migrate the HTTP pipeline to the new architecture in Azure Core V2. Refer to the following documentation for more details: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-rest-pipeline/documentation/core2.md

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?

For information on the Core V2 migration, refer to: https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/core/core-rest-pipeline/documentation/core2.md

This migration introduced the following two compatibility issues:

The phone numbers API uses relative URLs for nextLink values in endpoints that support paging, which is not supported out-of-the-box in the new client. While the current REST API guidelines state that they should be absolute URLs, this was introduced after the initial release of the API (for more details, see the PR discussion that introduced the change).
LROs in the phone numbers API have status-monitors given by the operation-location header; however, the @azure/core-lro implementation expects it to be in the legacy azure-asyncoperation header. While the current version of the guidelines discourage the later approach, they indicate that client code should look for both. For more details, refer to the Azure REST API guidelines.

In order to solve both issues, custom pipeline policies were implemented to make the HTTP responses have the form expected by the Core V2 libraries. These policies are workarounds and should be cleaned up once the incompatibilities are addressed.

Are there test cases added in this PR? (If not, why?)

Yes, the test cases for the policies implemented as workarounds for compatibility issues.

Provide a list of related PRs (if any)

@azure/communication-common Azure Core 2.0 Migration #20337

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 repository and link it here)
Added a changelog (if necessary)

Replace usages of the old @azure/core-http package in favor of the Core V2 packages. Also, update the autorest/typescript version to the latest beta to support generating clients using the new implementation.

The phoneNumbers API uses relative URLs for the `nextLink` value, which causes issues with the Azure Core V2 library. To address this, a pipeline policy was implemented to transform any relative `nextLink` to absolute URLs.

The phoneNumbers API uses LROs with status monitors, as described in the following URL https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#long-running-operations-with-status-monitor. However, the current implementation of the @azure/core-lro library only supports this pattern if the legacy `azure-asyncoperation` header is present in the response of the operation. Thus, as a workaround to this limitation, a pipeline policy was implemented that adds this header to the responses of LROs, setting it with the same value as the `operation-location` header.

danielortega-msft · 2022-03-03T20:37:29Z

FYI, the following issue was created to track the LRO incompatibility: #20647

sdk/communication/communication-phone-numbers/CHANGELOG.md

sdk/communication/communication-phone-numbers/src/phoneNumbersClient.ts

sdk/communication/communication-phone-numbers/src/utils/customPipelinePolicies.ts

jeremymeng · 2022-03-04T00:26:18Z

sdk/communication/communication-phone-numbers/src/phoneNumbersClient.ts

      },
    };

+    this.client = new PhoneNumbersGeneratedClient(url, {


A couple lines above, the code to handle userAgentOptions should be removed. https://github.com/Azure/autorest.typescript/wiki/%60core-http%60-dependency-migration-to-%60core-client%60-%60core-rest-pipeline%60#remove-the-code-to-handle-useragentoptions

Thanks, I wasn't aware of that guide. I will update accordingly!

...rowsers/phonenumbersclient__get_phone_number/recording_can_get_a_purchased_phone_number.json

...rsclient__lro__purchase_and_release_aad/recording_can_purchase_and_release_a_phone_number.js

...nenumbersclient__lro__search/recording_can_search_for_1_available_phone_number_by_default.js

...mbersclient__lro__search_aad/recording_can_search_for_1_available_phone_number_by_default.js

...gs/node/phonenumbersclient__lro__update/recording_can_update_a_phone_numbers_capabilities.js

sdk/communication/communication-phone-numbers/src/phoneNumbersClient.ts

sdk/communication/communication-phone-numbers/src/utils/customPipelinePolicies.ts

sdk/communication/communication-phone-numbers/test/internal/customPipelinePolicies.spec.ts

danielortega-msft · 2022-03-04T17:52:12Z

@lucasrsant the phone number that is present in the recordings is actually the sanitized value. We even explicitly use it as a fake number in one of our tests: https://github.com/danielortega-msft/azure-sdk-for-js/blob/0b5233eec003cf06439612349d9023bf50fee51a/sdk/communication/communication-phone-numbers/test/public/get.spec.ts#L37.

You can check in the recorded client configuration that the actual phone numbers are replaced with it.

const replaceableVariables: { [k: string]: string } = {
  COMMUNICATION_LIVETEST_STATIC_CONNECTION_STRING: "endpoint=https://endpoint/;accesskey=banana",
  INCLUDE_PHONENUMBER_LIVE_TESTS: "false",
  SKIP_UPDATE_CAPABILITIES_LIVE_TESTS: "false",
  COMMUNICATION_ENDPOINT: "https://endpoint/",
  AZURE_CLIENT_ID: "SomeClientId",
  AZURE_CLIENT_SECRET: "azure_client_secret",
  AZURE_TENANT_ID: "SomeTenantId",
  AZURE_PHONE_NUMBER: "+14155550100",
  COMMUNICATION_SKIP_INT_PHONENUMBERS_TESTS: "false",
};

Address the following feedback from the PR: - Remove the logic to generate the user agent - Specify the new major version of Azure Core used - Add integration tests between the PhoneNumbersClient and the custom policies implemented

danielortega-msft · 2022-03-04T20:45:59Z

@jeremymeng @Automagically @lucasrsant I updated the PR based on your comments, can you please do another review?

deyaaeldeen · 2022-03-05T02:05:00Z

FYI, the following issue was created to track the LRO incompatibility: #20647

The LRO bug has been fixed and I will make a release on Monday, 3/7.

sdk/communication/communication-phone-numbers/test/internal/headers.spec.ts

azure-sdk · 2022-03-07T19:04:31Z

API changes have been detected in @azure/communication-phone-numbers. You can review API changes here

API changes

- export interface PhoneNumbersClientOptions extends PipelineOptions { }
+ export interface PhoneNumbersClientOptions extends CommonClientOptions { }

jeremymeng

Looks good to me! I only have some minor comments.

jeremymeng · 2022-03-07T19:19:03Z

sdk/communication/communication-phone-numbers/swagger/README.md

 skip-enum-validation: true
 title: Phone Numbers Client
 v3: true
+use-core-v2: true


nit: use-core-v2 is true now by default so this is not needed.

jeremymeng · 2022-03-07T19:20:54Z

sdk/communication/communication-phone-numbers/src/utils/customPipelinePolicies.ts

+ *
+ * For more information on the LRO guidelines, refer to: https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#long-running-operations-with-status-monitor
+ */
+export const phoneNumbersLroPolicy: PipelinePolicy = {


maybe create a workitem to track removing this workaround when the LRO fix is available.

jeremymeng · 2022-03-07T19:28:29Z

sdk/communication/communication-phone-numbers/src/generated/src/phoneNumbersClientContext.ts

+      requestContentType: "application/json; charset=utf-8"
+    };
+
+    const packageDetails = `azsdk-js-azure-communication-phone-numbers/1.2.0-beta.2`;


The azure after azsdk-js- is probably from the package-name in swagger/README.md. Could you please use @azure/communication-phone-numbers instead?

danielortega-msft · 2022-03-07T19:52:44Z

Looks good to me! I only have some minor comments.

Thanks for the feedback @jeremymeng. I will merge this PR for now and, once the LRO fix is released, I will create a new one addressing your comments.

* Migrate to Azure Core V2 Replace usages of the old @azure/core-http package in favor of the Core V2 packages. Also, update the autorest/typescript version to the latest beta to support generating clients using the new implementation. * Fix purchased phone numbers pagination The phoneNumbers API uses relative URLs for the `nextLink` value, which causes issues with the Azure Core V2 library. To address this, a pipeline policy was implemented to transform any relative `nextLink` to absolute URLs. * Workaround for LRO polling in phone-numbers sdk The phoneNumbers API uses LROs with status monitors, as described in the following URL https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#long-running-operations-with-status-monitor. However, the current implementation of the @azure/core-lro library only supports this pattern if the legacy `azure-asyncoperation` header is present in the response of the operation. Thus, as a workaround to this limitation, a pipeline policy was implemented that adds this header to the responses of LROs, setting it with the same value as the `operation-location` header. * Fix skip condition for update capabilities tests * Fix purchase and release tests, update recordings * Add changelog entry * Test integration of client and custom policies Address the following feedback from the PR: - Remove the logic to generate the user agent - Specify the new major version of Azure Core used - Add integration tests between the PhoneNumbersClient and the custom policies implemented * Run all tests * Add missing license header, lint code * Make `PhoneNumbersClientOptions` extend `CommonClientOptions` * Fix metadata paths

danielortega-msft added 6 commits February 25, 2022 14:27

Migrate to Azure Core V2

2bf89f8

Replace usages of the old @azure/core-http package in favor of the Core V2 packages. Also, update the autorest/typescript version to the latest beta to support generating clients using the new implementation.

Fix purchased phone numbers pagination

ab10cd1

The phoneNumbers API uses relative URLs for the `nextLink` value, which causes issues with the Azure Core V2 library. To address this, a pipeline policy was implemented to transform any relative `nextLink` to absolute URLs.

Fix skip condition for update capabilities tests

791c6da

Fix purchase and release tests, update recordings

ee63b50

Add changelog entry

0b5233e

danielortega-msft marked this pull request as ready for review March 3, 2022 17:46

danielortega-msft requested review from RoyHerrod, bterlson, danielav7, jeremymeng, lucasrsant, miguhern, ramya-rao-a, whisper6284 and xirzec as code owners March 3, 2022 17:46

automagicaly reviewed Mar 3, 2022

View reviewed changes

jeremymeng reviewed Mar 4, 2022

View reviewed changes

lucasrsant suggested changes Mar 4, 2022

View reviewed changes

Test integration of client and custom policies

13eb10f

Address the following feedback from the PR: - Remove the logic to generate the user agent - Specify the new major version of Azure Core used - Add integration tests between the PhoneNumbersClient and the custom policies implemented

Run all tests

9d051ce

danielortega-msft requested review from automagicaly, jeremymeng and lucasrsant and removed request for ramya-rao-a March 4, 2022 20:53

Add missing license header, lint code

536dcb3

automagicaly approved these changes Mar 4, 2022

View reviewed changes

lucasrsant approved these changes Mar 7, 2022

View reviewed changes

jeremymeng reviewed Mar 7, 2022

View reviewed changes

sdk/communication/communication-phone-numbers/test/internal/headers.spec.ts Outdated Show resolved Hide resolved

Make PhoneNumbersClientOptions extend CommonClientOptions

c434ffe

Fix metadata paths

46e8a0a

jeremymeng approved these changes Mar 7, 2022

View reviewed changes

danielortega-msft merged commit 512ec36 into Azure:main Mar 7, 2022

danielortega-msft deleted the feature/core-v2-phonenumbers branch March 7, 2022 19:57

This was referenced Mar 7, 2022

[Communication] - phone-numbers - Azure Core V2 migration cleanup #20696

Merged

[Communication] - phone-numbers - Prepare communication-phone-numbers for release #20759

Merged

[communication-short-codes] Migrate to Core v2 #19043

Merged

petrsvihlik mentioned this pull request Mar 17, 2022

[Communication]Move the Communication packages to use core-rest-pipeline #15814

Closed

arifibrahim4 mentioned this pull request Apr 25, 2022

Feature/sms core v2 and test recoder migration #21328

Merged

4 tasks

danielortega-msft mentioned this pull request May 2, 2022

Update the @azure/communication-phone-numbers package to use the new @azure/communication-common #20379

Closed

[Communication] - phone-numbers - Core V2 migration #20633

[Communication] - phone-numbers - Core V2 migration #20633

Uh oh!

Conversation

danielortega-msft commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

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)

Checklists

Uh oh!

danielortega-msft commented Mar 3, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremymeng Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

danielortega-msft Mar 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielortega-msft commented Mar 4, 2022

Uh oh!

danielortega-msft commented Mar 4, 2022

Uh oh!

deyaaeldeen commented Mar 5, 2022

Uh oh!

Uh oh!

azure-sdk commented Mar 7, 2022

Uh oh!

jeremymeng left a comment

Choose a reason for hiding this comment

Uh oh!

jeremymeng Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jeremymeng Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jeremymeng Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

danielortega-msft commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

danielortega-msft commented Mar 3, 2022 •

edited

Loading