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.

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

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

Merged
merged 25 commits into from
Jan 15, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions sdk/servicebus/service-bus/review/service-bus.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
```ts

import { AmqpMessage } from '@azure/core-amqp';
import { ConnectionConfig } from '@azure/core-amqp';
import { DataTransformer } from '@azure/core-amqp';
import { delay } from '@azure/core-amqp';
import { Delivery } from 'rhea-promise';
import Long from 'long';
import { MessagingError } from '@azure/core-amqp';
import { RetryOptions } from '@azure/core-amqp';
import { SharedKeyCredential } from '@azure/core-amqp';
import { TokenCredential } from '@azure/core-amqp';
import { TokenType } from '@azure/core-amqp';
import { WebSocketImpl } from 'rhea-promise';
Expand Down Expand Up @@ -64,8 +66,8 @@ export interface OnMessage {
// @public
export class QueueClient implements Client {
close(): Promise<void>;
createReceiver(receiveMode: ReceiveMode, sessionOptions: SessionReceiverOptions): SessionReceiver;
createReceiver(receiveMode: ReceiveMode): Receiver;
createReceiver(receiveMode: ReceiveMode, sessionOptions: SessionReceiverOptions): SessionReceiver;
createSender(): Sender;
readonly entityPath: string;
static getDeadLetterQueuePath(queueName: string): string;
Expand Down Expand Up @@ -150,8 +152,9 @@ export class Sender {

// @public
export class ServiceBusClient {
constructor(connectionString: string, options?: ServiceBusClientOptions);
constructor(config: ConnectionConfig, credential: SharedKeyCredential | TokenCredential, options?: ServiceBusClientOptions);
close(): Promise<any>;
static createFromConnectionString(connectionString: string, options?: ServiceBusClientOptions): ServiceBusClient;
createQueueClient(queueName: string): QueueClient;
createSubscriptionClient(topicName: string, subscriptionName: string): SubscriptionClient;
createTopicClient(topicName: string): TopicClient;
Expand Down
90 changes: 55 additions & 35 deletions sdk/servicebus/service-bus/src/serviceBusClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
ConnectionConfig,
DataTransformer,
TokenCredential,
SharedKeyCredential
SharedKeyCredential,
isTokenCredential
} from "@azure/core-amqp";
import { SubscriptionClient } from "./subscriptionClient";

Expand Down Expand Up @@ -56,23 +57,69 @@ export class ServiceBusClient {
*/
private _context: ConnectionContext;

/**
* Creates a ServiceBusClient for the Service Bus Namespace represented in the given connection
* string.
* @param connectionString - Connection string of the form
* 'Endpoint=sb://my-servicebus-namespace.servicebus.windows.net/;SharedAccessKeyName=my-SA-name;SharedAccessKey=my-SA-key'
* @param options Options to control ways to interact with the
* Service Bus Namespace.
* @returns ServiceBusClient
*/
constructor(connectionString: string, options?: ServiceBusClientOptions);

/**
* Instantiates a ServiceBusClient to interact with a Service Bus Namespace.
*
* @constructor
* @param {ConnectionConfig} config - The connection configuration needed to connect to the
* Service Bus Namespace.
* @param {TokenCredential} [tokenCredential] - SharedKeyCredential object or your
* @param host - The host name for the Service Bus namespace. This is likely to be similar to
* <yournamespace>.servicebus.windows.net
* @param credential - SharedKeyCredential object or your
* credential that implements the TokenCredential interface.
* @param {ServiceBusClientOptions} - Options to control ways to interact with the Service Bus
* @param options - Options to control ways to interact with the Service Bus
* Namespace.
*/
private constructor(
config: ConnectionConfig,
credential: SharedKeyCredential | TokenCredential,
constructor(host: string, credential: TokenCredential, options?: ServiceBusClientOptions);
constructor(
hostOrConnectionString: string,
credentialOrServiceBusClientOptions?: TokenCredential | ServiceBusClientOptions,
options?: ServiceBusClientOptions
) {
let config;
let credential;
let connectionString;

if (!options) options = {};

if (hostOrConnectionString == undefined || typeof hostOrConnectionString !== "string") {
throw new Error("Input parameter of host or connection string must be a string.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The above checks are not needed. ConnectionConfig.create() ensures that String() is called on the given connection string. If the connection string thus stringified doesnt meet the requirements of a connection string, then appropriate errors are thrown from ConnectionConfig.create().

I would suggest the below for handling the overloads

if (!isTokenCredential(credentialOrServiceBusClientOptions)) {
    // assume that first overload is used & create the ConnectionConfig
} else {
    // assume that second overload is used & create the ConnectionConfig
}
// create the connection context

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for converting to String is because if it's a hostname then we use a .endsWith() check in here

Copy link
Contributor

Choose a reason for hiding this comment

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

You should still be able to use hostOrConnectionString.endsWith(), TS should not complain as the type is already set to string in the constructor signature.

If you want to avoid the case of runtime error for when a bad type is passed, then you can use hostOrConnectionString = String(hostOrConnectionString). Since then endsWith() check is needed only in the case of TokenCredential, you can move this inside the if/else block as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any reason why we want to convert it to type String and not use .toString()? If i remember correctly, I think @bterlson had once discussed, advised about not doing so and using string where possible.
And yes, the conversion was to help with both compile and run time scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

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

My intention here is to simplify the logic to handle the constructor overloads.
Currently, we are adding multiple lines of code to ensure that hostOrConnectionString is indeed a string and that it has = in it. We can avoid the need for this and the future tax of maintaining them as well.

For example, when the constructor overload is using connection string, we shouldnt need to do any checks on hostOrConnectionString as that work is offloaded to the downstream ConnectionConfig.create().

When the constructor overload for credential is being used, us using String() will ensure that if user had provided an object with .toString() defined, then it gets called and used correctly. If the passed object does not have .toString() on it, then the connection to the service would fail eventually with the service providing an error message hinting that the provided host name is not valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's a trade off with quality of expected user experience here possibly - for instance, if a user is passing in the hostname correctly and receives a connection string malformed error, wouldn't that be confusing / inappropriate?

Copy link
Member Author

@ramya0820 ramya0820 Jan 14, 2020

Choose a reason for hiding this comment

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

For above, let's say the inputs are passed along with a malformed / incorrect / unsupported type of credentials.
Current changes address these cases.
Offloading the check to service or core-amqp, which expect a connection string, would make it not possible to do so. (So I'd keep the current changes unless we want or are okay that user receives the connection string related error, in these scenarios - I had added tests reflecting the behavior current changes emulate, can take a look at those. Will need to update those as well, based on what we decide to do.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use a different signature for the credential based constructor - should likely help with both simplification/maintenance and user ex concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. 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).
  2. 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!


if (!isTokenCredential(credentialOrServiceBusClientOptions)) {
// connectionString and options based constructor was invoked
connectionString = hostOrConnectionString;
config = ConnectionConfig.create(connectionString);
config.webSocket = options && options.webSocket;
config.webSocketEndpointPath = "$servicebus/websocket";
config.webSocketConstructorOptions = options && options.webSocketConstructorOptions;

// Since connectionstring was passed, create a SharedKeyCredential
credential = new SharedKeyCredential(config.sharedAccessKeyName, config.sharedAccessKey);

ConnectionConfig.validate(config);
} else {
if (credentialOrServiceBusClientOptions == undefined) {
throw new Error("Input parameter token credential must be defined.");
}
credential = credentialOrServiceBusClientOptions as TokenCredential;

if (!hostOrConnectionString.endsWith("/")) {
hostOrConnectionString += "/";
}
connectionString = `Endpoint=sb://${hostOrConnectionString};SharedAccessKeyName=defaultKeyName;SharedAccessKey=defaultKeyValue;`;
config = ConnectionConfig.create(connectionString);
}

this.name = config.endpoint;
this._context = ConnectionContext.create(config, credential, options);
}
Expand Down Expand Up @@ -149,33 +196,6 @@ export class ServiceBusClient {
}
}

/**
* Creates a ServiceBusClient for the Service Bus Namespace represented in the given connection
* string.
* @param {string} connectionString - Connection string of the form
* 'Endpoint=sb://my-servicebus-namespace.servicebus.windows.net/;SharedAccessKeyName=my-SA-name;SharedAccessKey=my-SA-key'
* @param {ServiceBusClientOptions} [options] Options to control ways to interact with the
* Service Bus Namespace.
* @returns {ServiceBusClient}
*/
static createFromConnectionString(
connectionString: string,
options?: ServiceBusClientOptions
): ServiceBusClient {
const config = ConnectionConfig.create(connectionString);

config.webSocket = options && options.webSocket;
config.webSocketEndpointPath = "$servicebus/websocket";
config.webSocketConstructorOptions = options && options.webSocketConstructorOptions;

// Since connectionstring was passed, create a SharedKeyCredential
const credential = new SharedKeyCredential(config.sharedAccessKeyName, config.sharedAccessKey);

ConnectionConfig.validate(config);

return new ServiceBusClient(config, credential, options);
}

// /**
// * Creates a ServiceBusClient for the Service Bus Namespace represented by the given host using
// * the given TokenProvider.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ async function RunTest(
maxConcurrentCalls: number,
messages: number
): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

const client = ns.createQueueClient(entityPath);
const receiver = client.createReceiver(ReceiveMode.receiveAndDelete);
Expand Down
2 changes: 1 addition & 1 deletion sdk/servicebus/service-bus/test/perf/service-bus/send.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ async function RunTest(
maxInflight: number,
messages: number
): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

const client = ns.createQueueClient(entityPath);
const sender = client.createSender();
Expand Down
6 changes: 3 additions & 3 deletions sdk/servicebus/service-bus/test/serviceBusClient.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ describe("Create ServiceBusClient and Queue/Topic/Subscription Clients #RunInBro
});

it("Creates an Namespace from a connection string", function(): void {
sbClient = ServiceBusClient.createFromConnectionString(
sbClient = new ServiceBusClient(
"Endpoint=sb://a;SharedAccessKeyName=b;SharedAccessKey=c;EntityPath=d"
);
sbClient.should.be.an.instanceof(ServiceBusClient);
should.equal(sbClient.name, "sb://a/", "Name of the namespace is different than expected");
});

it("Creates clients after coercing name to string", function(): void {
sbClient = ServiceBusClient.createFromConnectionString(
sbClient = new ServiceBusClient(
"Endpoint=sb://a;SharedAccessKeyName=b;SharedAccessKey=c;EntityPath=d"
);
const queueClient = sbClient.createQueueClient(1 as any);
Expand Down Expand Up @@ -88,7 +88,7 @@ describe("Errors with non existing Namespace #RunInBrowser", function(): void {
let sbClient: ServiceBusClient;
let errorWasThrown: boolean;
beforeEach(() => {
sbClient = ServiceBusClient.createFromConnectionString(
sbClient = new ServiceBusClient(
"Endpoint=sb://a;SharedAccessKeyName=b;SharedAccessKey=c;EntityPath=d"
);
errorWasThrown = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async function main(): Promise<void> {
}

async function sendReceiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

const clients = [];
const senders = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ async function main(): Promise<void> {
}

async function sendReceiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

const clients = [];
const senders = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async function main(): Promise<void> {
}

async function sendMessage(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -54,7 +54,7 @@ async function sendMessage(): Promise<void> {
}

async function receiveMessage(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ async function main(): Promise<void> {
}

async function sendMessage(messageId: string): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -57,7 +57,7 @@ async function sendMessage(messageId: string): Promise<void> {
}

async function receiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ async function main(): Promise<void> {
}

async function sendMessage(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -55,7 +55,7 @@ async function sendMessage(): Promise<void> {
}

async function receiveMessage(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ async function main(): Promise<void> {
}

async function sendMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -70,7 +70,7 @@ async function sendMessages(): Promise<void> {
}

async function receiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ async function main(): Promise<void> {
}

async function sendMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -72,7 +72,7 @@ async function sendMessages(): Promise<void> {
}

async function receiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async function main(): Promise<void> {
}

async function sendMessage(sessionId: string): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -55,7 +55,7 @@ async function sendMessage(sessionId: string): Promise<void> {
}

async function receiveMessage(sessionId: string): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ async function main(): Promise<void> {
}

async function sendMessage(sessionId: string): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -57,7 +57,7 @@ async function sendMessage(sessionId: string): Promise<void> {
}

async function receiveMessage(sessionId: string): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ async function main(): Promise<void> {
}

async function setGetSessionState(sessionId: string): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async function main(): Promise<void> {
}

async function sendReceiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);

try {
while (!isJobDone) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ async function main(): Promise<void> {
}

async function sendMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);
try {
const sender = client.createSender();
Expand All @@ -63,7 +63,7 @@ async function sendMessages(): Promise<void> {
}

async function receiveMessages(): Promise<void> {
const ns = ServiceBusClient.createFromConnectionString(connectionString);
const ns = new ServiceBusClient(connectionString);
const client = ns.createQueueClient(queueName);

try {
Expand Down
2 changes: 1 addition & 1 deletion sdk/servicebus/service-bus/test/utils/testUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,5 +501,5 @@ export function getNamespace(serviceBusConnectionString: string): string {

export function getServiceBusClient(): ServiceBusClient {
const env = getEnvVars();
return ServiceBusClient.createFromConnectionString(env[EnvVarKeys.SERVICEBUS_CONNECTION_STRING]);
return new ServiceBusClient(env[EnvVarKeys.SERVICEBUS_CONNECTION_STRING]);
}