[Service Bus] Update constructors to add overloads and use Azure Identity #5436

ramya0820 · 2019-10-07T21:39:43Z

For more context refer to #4262

Verified build and local smoke test run of live tests

ramya-rao-a · 2019-10-07T22:29:57Z

I'll hold back on reviewing this PR until we rebase/merge from master after #5324 gets merged

…e/azure-sdk-for-js into issue-4262

ramya0820 · 2020-01-02T19:45:45Z

Updated PR with changes from #5324 - verified that build and tests are running fine on local. @ramya-rao-a @chradek

ramya-rao-a

In Track2, we do not want to expose SharedKeyCredential and ConnectionConfig from our client constructors. Use the EventHubProducerClient constructor overloads in the Event Hubs library as reference if needed.

ramya0820 · 2020-01-03T00:42:59Z

In Track2, we do not want to expose SharedKeyCredential and ConnectionConfig from our client constructors. Use the EventHubProducerClient constructor overloads in the Event Hubs library as reference if needed.

Updated PR
Looks like the way we construct ConnectionCOnfig internally is by manually constructing the connection string.
This seems specific to Event Hubs and unsure if the same approach can be used for SB.

sdk/servicebus/service-bus/src/serviceBusClient.ts

sdk/servicebus/service-bus/review/service-bus.api.md

sdk/servicebus/service-bus/src/serviceBusClient.ts

ramya-rao-a · 2020-01-14T02:33:52Z

There was a test for custom token provider which was commented in the track 2 branch because the createFromTokenProvider was commented in this branch.

Now that we have the constructor overload that can take a custom token provider (i.e a custom TokenCredential), we should re-enable the above test as well.

ramya-rao-a

Few pending items other than the ongoing discussion on the constructor overloads:

The feature/service-bus-track-2 branch has the latest changes from master now and therefore is causing quite a few merge conflicts in this PR. Please update your branch with the latest from feature/service-bus-track-2.
Re-enable the commented test in as mentioned in [Service Bus] Update constructors to add overloads and use Azure Identity #5436 (comment)

ramya0820 · 2020-01-14T18:19:29Z

Had a question on about which TokenCredential to use in place of the SasTokenProvider from amqp-common that was being used previously. https://github.com/Azure/azure-sdk-for-js/pull/6770/files#r366494233 @ramya-rao-a

chradek · 2020-01-14T18:11:48Z

sdk/servicebus/service-bus/review/service-bus.api.md

@@ -150,8 +150,9 @@ export class Sender {

 // @public
 export class ServiceBusClient {
+    constructor(connectionString: string, options?: ServiceBusClientOptions);
+    constructor(credential: TokenCredential, host: string, options?: ServiceBusClientOptions);


I think the ordering of parameters here should be switched for consistency with event hubs.

constructor(host: string, credential: TokenCredential, options?: ServiceBusClientOptions);

This also matches the sample in the typescript design guidelines.

Oh, could you point to which guideline is being considered for this?

Because I found the following that seems similar / applicable for hostOrConnectionString usage

// bad example class ExampleClient { constructor (connectionString: string, options: ExampleClientOptions); constructor (url: string, options: ExampleClientOptions); constructor (urlOrCS: string, options: ExampleClientOptions) { // have to dig into the first parameter to see whether its // a url or a connection string. Not ideal. } }

Link was hidden in the word 'guidelines' :)
https://azure.github.io/azure-sdk/typescript_design.html#ts-apisurface-serviceclient

See the Client API example.

// client constructors have overloads for handling different
// authentication schemes.
constructor(connectionString: string, options?: ServiceClientOptions);
constructor(host: string, credential: TokenCredential, options?: ServiceClientOptions);

The reason the example you posted is considered bad is because the type signatures for both constructors are exactly the same (string, options). In our case though we have 2 unique signatures (string, options) and (string, TokenCredential, options)

Unable to reply to other comment for some reason :?
Yes, EventHubs impl seems to have the different concerns addressed. The type of error being thrown is useful to note as well (TypeError as opposed to simple Error)
I just have the guideline aspect clarification pending - ok to be consistent with EventHubs if that's what we want to do
Thoughts? @ramya-rao-a @bterlson

Personally, I think the example called out on the guidelines page resonates with my initial thoughts on switching the parameters - also because credential based constructor sounded more right too, than host based. (Going by the first parameter to pass in).

Ok, the other observation I had was that options parameter being optional - seemed out of place to rely on type of to decide which constructor to use at first. So without that it can come down to two parameter constructor :?

Like, JS in general isn't strict on parameter numbers to be passed in.
So I think the one usecase that I had brought up earlier and seems to not be handled by EventHubs is if user invokes the host and credential based constructor as
(hostName, undefined, options) - then it looks like the error user would get would be about having to pass in a correctly formatted connection string.

To summarize the thread here - the user experience for when credential is passed in as undefined is the one that seems unaddressed.

Let's take a call on if we want to address that as a bug and fix it here or not - and if we want to still go ahead with mirroring EventHubs constructors for most part.

cc @ramya-rao-a @chradek
@bterlson @AlexGhiondea

The situation where a user passed undefined for the credential but still passes the options after it seems very unlikely. I think you could make the argument that if they're explicitly trying to not pass in a TokenCredential, then they would want a connection string (so the error seems relevant). But, I don't feel too strongly since they would be going against the types and docs in this case (I'm glad we give some error).

With regards to the parameter order for the constructors, my vote is for consistency. Since event-hubs is already GA, changing the order there would need a major version bump. I see one of our goals as providing a consistent experience across the packages we own (when possible) to lower the learning curve for users going from one package to another.

Fair point, updated parameters ordering to keep up with consistency as if we decide to change, we can do it together

Agree that explicitly passing in undefined would map more to connection string based constructor.
But if say a user is instantiating the credentials programmatically, they could run into an undefined object.
Event Hubs seems a bit different in that it accepts event hub name in addition to connection string in Event Hubs SDK. So it seems a little more relevant to Event Hubs as the err msg can be confusing to troubleshoot. (Also I wonder how long it might be before we bump version, maybe that would happen at same time as when Track 2 SB would go GA, but I digress :) )

And.. if we are of pinion that this a not so ideal state afterall, then we could have not so ideal string processing to guard against the missing/undefined token credentials case in Service Bus possibly. Let me know. For now haven't included those checks since we didn't want those. @ramya-rao-a Ok if we choose to handle it all together later as well or file a new issue?
Because it seems like we might be regressing from Track 1 if we are no longer able to surface the error we used to be able to - "'credentials' is a required parameter and must be an instance of ...

chradek · 2020-01-14T18:30:38Z

sdk/servicebus/service-bus/src/serviceBusClient.ts

+    let isConnectionStringConstructorInvoked = true;
+    if (hostOrConnectionString.indexOf("=") < 0) {
+      isConnectionStringConstructorInvoked = false;
+    }


I'm just catching up here and I might have missed some things as changes have been made.
1st, I remarked on this in the API file but I do think we want to keep connectionString and host as the first parameter in their respective constructors to be consistent with our other SDKs/guidelines.

I think the original comment was about the checks being done on hostOrConnectionString.

if ( hostOrConnectionString == undefined || hostOrConnectionString.toString == undefined || hostOrConnectionString.toString().trim() == "" ) { throw new Error( "Input parameter of host or connection string must be defined and coercible to string." ); }

I think @ramya-rao-a 's had 2 points:

We can ensure hostOrConnectionString is a string by wrapping it in String: String(hostOrConnectionString)
Note that this is different from new String(hostOrConnectionString), which is not what you want (and what I think @bterlson would warn to stay away from).

There are other places where hostOrConnectionString is validated so this check isn't needed.

The EventHubClient constructor shows both of these aspects off (only linking to one line here but look at the rest of the function): https://github.com/Azure/azure-sdk-for-js/blob/master/sdk/eventhub/event-hubs/src/impl/eventHubClient.ts#L339

Event Hubs is a bit more complicated since it has 3 overloads instead of 2, so service bus would be even simpler.

I think the Event Hubs constructor would throw helpful errors for the cases that were being checked before, but if there's something being missed let me know!

…e/azure-sdk-for-js into issue-4262

sdk/servicebus/service-bus/test/utils/aadUtils.ts

sdk/servicebus/service-bus/test/serviceBusClient.spec.ts

ramya-rao-a · 2020-01-15T00:56:35Z

sdk/servicebus/service-bus/test/serviceBusClient.spec.ts

+    should.equal(errorWasThrown, true, "Error thrown flag must be true");
+  });
+
+  if (isNode) {


What is the reason for the isNode check here?

That's because EnvironmentCredential is not supported in browser, so only a select subset of tests can be run in browser mode.

Carrying forward Q I had from other somewhat related comment - is SharedKeyCredential not going to be used and be removed from core-amqp? If not, was it supposed to work in browser?

Regarding the question on SharedKeyCredential, see #6770 (comment)

Looks like among all the credentials from azure/identity, only the interactive credential works in browser which of course we can't use in CI.

There is ongoing work to change that. For now, I believe all libraries using azure/identity are falling under this case where the credentials from the azure/identity library are not testable in browser in CI.

ramya-rao-a

Looks good. Pending items:

Address the question on the need for isNode check in the test. See [Service Bus] Update constructors to add overloads and use Azure Identity #5436 (comment)
Merge from feature/service-bus-track-2 to ensure the changes from the PR [Service Bus] Update to use latest core-amqp #6861 works well with the changes in this PR
Trigger a round of live tests after addressing the above.

…e/azure-sdk-for-js into issue-4262

ramya0820 · 2020-01-15T02:43:32Z

/azp run js - servicebus - tests

azure-pipelines · 2020-01-15T02:43:42Z

Azure Pipelines successfully started running 1 pipeline(s).

ramya-rao-a · 2020-01-15T06:23:30Z

sdk/servicebus/service-bus/src/core/batchingReceiver.ts

@@ -92,7 +92,7 @@ export class BatchingReceiver extends MessageReceiver {
        const sessionError = context.session && context.session.error;
        let error: Error | MessagingError;
        if (sessionError) {
-          error = translate(sessionError);
+          error = translate(sessionError) as MessagingError;


This casting is not required. Same for a few lines below.

ramya-rao-a · 2020-01-15T06:25:25Z

sdk/servicebus/service-bus/test/README.md

@@ -2,7 +2,9 @@

 - Run `npm i` to install all the dependencies of this project. This is a one time task.
 - The tests expect a series of queues, topics and subscriptions to exist in a single Service Bus instance.


After merging the feature/service-bus-atom branch, all tests now create the entities they need. So the above statement is no longer true.

Suggested change

- The tests expect a series of queues, topics and subscriptions to exist in a single Service Bus instance.

- The tests will create the required queue, topic and subscription in the Service Bus namespace you provide using environment variables.

ramya-rao-a · 2020-01-15T06:26:21Z

sdk/servicebus/service-bus/test/README.md

-  The connection string for the service bus should be in the environment variable `SERVICEBUS_CONNECTION_STRING`.
+  For Node environment, the connection string for the service bus should be in the environment variable `SERVICEBUS_CONNECTION_STRING`.
+
+  For browser environment, the connection string for the service bus should be in the environment variable `SERVICEBUS_CONNECTION_STRING_BROWSER`.

  Note that the tests will empty the messages in these entities to get a clean start before running each test.


After merging the feature/service-bus-atom branch, all tests now create the entities they need. So this note and the table following it can be removed.

Similarly the sample content for the .env file should be cleaned up as well

ramya-rao-a · 2020-01-15T06:28:42Z

sdk/servicebus/service-bus/package.json

@@ -93,6 +92,7 @@
  },
  "devDependencies": {
    "@azure/eslint-plugin-azure-sdk": "^2.0.1",
+    "@azure/ms-rest-nodeauth": "^0.9.2",


Now that aadutils.ts file is deleted, we don't need @azure/ms-rest-nodeauth in tests either right?

agree, updated

ramya-rao-a · 2020-01-15T18:00:37Z

On second thoughts, the changes done to the readme file for test in this PR other than the ones related to changing AAD-* to AZURE-*, should be done in the master branch.

This reverts commit 51af115.

ramya0820 · 2020-01-15T19:29:13Z

Agree on README related changes to be on master. Have opened #6974 and addressed the comments and more changes on there.

* Pin core-amqp version to preview.1 * Update lock file * Fix merge conflicts * [Service Bus] Update to use core-amqp (#5324) * Use core-amqp in atom management client, comment tests that use token provider * [Service Bus] Update to use latest core-amqp (#6861) * [Service Bus] Update constructors to add overloads and use Azure Identity (#5436) * sync-versions: rhea-promise and @azure/core-amqp for service-bus * resolve merge conflicts appropriately * API Report File for "@Azure/service-bus" * Address feedback * SERVICEBUS_CONNECTION_STRING_BROWSER remove from karma.conf * update pnpm-lock file * update API Report File for "@Azure/service-bus" * remove rhea and rhea-promise dependencies * add "rhea-promise" back Co-authored-by: Ramya Rao <[email protected]> Co-authored-by: ramya0820 <[email protected]>

Update constructor

2d4f970

ramya0820 requested review from chradek, KarishmaGhiya, mikeharder, ramya-rao-a and ShivangiReja as code owners October 7, 2019 21:39

ramya0820 self-assigned this Oct 7, 2019

ramya0820 changed the base branch from master to feature/service-bus-track-2 January 2, 2020 19:11

Ramya Raja added 2 commits January 2, 2020 11:27

Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…

fa13083

…e/azure-sdk-for-js into issue-4262

Update api.md file

b7c5198

ramya-rao-a suggested changes Jan 2, 2020

View reviewed changes

ramya-rao-a removed request for KarishmaGhiya, mikeharder and ShivangiReja January 2, 2020 20:02

Replicate Event Hubs client creation

afc8689

Clean up

3bcb540

ramya0820 force-pushed the issue-4262 branch from b0ab1c6 to 3bcb540 Compare January 3, 2020 00:47

ramya-rao-a reviewed Jan 3, 2020

View reviewed changes

sdk/servicebus/service-bus/src/serviceBusClient.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 3, 2020

View reviewed changes

sdk/servicebus/service-bus/src/serviceBusClient.ts Outdated Show resolved Hide resolved

Fix setting of options

99d2d57

ramya0820 requested a review from bterlson as a code owner January 3, 2020 22:33

Update docs

2b00a70

ramya-rao-a reviewed Jan 4, 2020

View reviewed changes

sdk/servicebus/service-bus/review/service-bus.api.md Outdated Show resolved Hide resolved

Ramya Raja added 2 commits January 8, 2020 15:39

Handle undefined input correctly

1c8760a

Update api.md file

9883ac9

ramya-rao-a reviewed Jan 9, 2020

View reviewed changes

sdk/servicebus/service-bus/src/serviceBusClient.ts Outdated Show resolved Hide resolved

Attempt adding tests

ced6b23

Update signature

115b53f

ramya0820 force-pushed the issue-4262 branch from 48a753e to 115b53f Compare January 14, 2020 03:38

ramya-rao-a suggested changes Jan 14, 2020

View reviewed changes

Enable streaming receiver test

9d0597b

chradek reviewed Jan 14, 2020

View reviewed changes

Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…

865b0d8

…e/azure-sdk-for-js into issue-4262

ramya-rao-a reviewed Jan 14, 2020

View reviewed changes

sdk/servicebus/service-bus/test/utils/aadUtils.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 14, 2020

View reviewed changes

sdk/servicebus/service-bus/test/utils/aadUtils.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 14, 2020

View reviewed changes

sdk/servicebus/service-bus/test/serviceBusClient.spec.ts Show resolved Hide resolved

Ramya Raja added 4 commits January 14, 2020 13:48

Remove unused util

c4a2267

Clean up comments

33ce753

Switch parameters

a749abd

Remove need for aadUtils.browser

d5b3eb0

ramya-rao-a reviewed Jan 15, 2020

View reviewed changes

Ramya Raja added 2 commits January 14, 2020 18:39

Clean up README file for tests

51af115

Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…

d0dcec0

…e/azure-sdk-for-js into issue-4262

ramya-rao-a suggested changes Jan 15, 2020

View reviewed changes

Ramya Raja added 2 commits January 15, 2020 11:02

Revert "Clean up README file for tests"

e05a944

This reverts commit 51af115.

Address comments

e9d9dfb

ramya-rao-a approved these changes Jan 15, 2020

View reviewed changes

ramya0820 merged commit 6947606 into Azure:feature/service-bus-track-2 Jan 15, 2020

ramya-rao-a mentioned this pull request Jan 23, 2020

[Service Bus] Update constructors to add overloads and use Azure Identity #4262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Service Bus] Update constructors to add overloads and use Azure Identity #5436

[Service Bus] Update constructors to add overloads and use Azure Identity #5436

ramya0820 commented Oct 7, 2019 •

edited

Loading

ramya-rao-a commented Oct 7, 2019

ramya0820 commented Jan 2, 2020

ramya-rao-a left a comment

ramya0820 commented Jan 3, 2020

ramya-rao-a commented Jan 14, 2020

ramya-rao-a left a comment

ramya0820 commented Jan 14, 2020

chradek Jan 14, 2020

ramya0820 Jan 14, 2020

chradek Jan 14, 2020

chradek Jan 14, 2020

ramya0820 Jan 14, 2020

ramya0820 Jan 14, 2020

ramya0820 Jan 14, 2020

chradek Jan 14, 2020

ramya0820 Jan 14, 2020 •

edited

Loading

chradek Jan 14, 2020

ramya-rao-a Jan 15, 2020 •

edited

Loading

ramya0820 Jan 15, 2020

ramya-rao-a Jan 15, 2020

ramya-rao-a left a comment

ramya0820 commented Jan 15, 2020

azure-pipelines bot commented Jan 15, 2020

ramya-rao-a Jan 15, 2020

ramya-rao-a Jan 15, 2020

ramya-rao-a Jan 15, 2020 •

edited

Loading

ramya-rao-a Jan 15, 2020

ramya0820 Jan 15, 2020

ramya-rao-a commented Jan 15, 2020

ramya0820 commented Jan 15, 2020

		@@ -2,7 +2,9 @@

		- Run `npm i` to install all the dependencies of this project. This is a one time task.
		- The tests expect a series of queues, topics and subscriptions to exist in a single Service Bus instance.

	- The tests expect a series of queues, topics and subscriptions to exist in a single Service Bus instance.
	- The tests will create the required queue, topic and subscription in the Service Bus namespace you provide using environment variables.

[Service Bus] Update constructors to add overloads and use Azure Identity #5436

[Service Bus] Update constructors to add overloads and use Azure Identity #5436

Conversation

ramya0820 commented Oct 7, 2019 • edited Loading

ramya-rao-a commented Oct 7, 2019

ramya0820 commented Jan 2, 2020

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya0820 commented Jan 3, 2020

ramya-rao-a commented Jan 14, 2020

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya0820 commented Jan 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya0820 Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya0820 commented Jan 15, 2020

azure-pipelines bot commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Jan 15, 2020

ramya0820 commented Jan 15, 2020

ramya0820 commented Oct 7, 2019 •

edited

Loading

ramya0820 Jan 14, 2020 •

edited

Loading

ramya-rao-a Jan 15, 2020 •

edited

Loading

ramya-rao-a Jan 15, 2020 •

edited

Loading