[Service Bus] Update constructors to add overloads and use Azure Identity #5436
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,8 @@ import { | |
| ConnectionConfig, | ||
| DataTransformer, | ||
| TokenCredential, | ||
| SharedKeyCredential, | ||
| isTokenCredential | ||
| } from "@azure/core-amqp"; | ||
| import { SubscriptionClient } from "./subscriptionClient"; | ||
|
|
||
|
|
@@ -56,23 +57,87 @@ 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 host - The host name for the Service Bus namespace. This is likely to be similar to | ||
| * <yournamespace>.servicebus.windows.net | ||
| * @param credential - credential that implements the TokenCredential interface. | ||
| * @param options - Options to control ways to interact with the Service Bus | ||
| * Namespace. | ||
| */ | ||
| 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 || | ||
| hostOrConnectionString.toString == undefined || | ||
| hostOrConnectionString.toString().trim() == "" | ||
| ) { | ||
| throw new Error( | ||
| "Input parameter of host or connection string must be defined and coercible to string." | ||
| ); | ||
| } | ||
| hostOrConnectionString = hostOrConnectionString.toString(); | ||
|
|
||
| let isConnectionStringConstructorInvoked = true; | ||
| if (hostOrConnectionString.indexOf("=") < 0) { | ||
| isConnectionStringConstructorInvoked = false; | ||
| } | ||
|
There was a problem hiding this comment. The above checks are not needed. 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 contextThere was a problem hiding this comment. The reason for converting to String is because if it's a hostname then we use a .endsWith() check in here There was a problem hiding this comment. You should still be able to use If you want to avoid the case of runtime error for when a bad type is passed, then you can use There was a problem hiding this comment. Any reason why we want to convert it to type There was a problem hiding this comment. My intention here is to simplify the logic to handle the constructor overloads. For example, when the constructor overload is using connection string, we shouldnt need to do any checks on 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. There was a problem hiding this comment. 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? There was a problem hiding this comment. For above, let's say the inputs are passed along with a malformed / incorrect / unsupported type of credentials. There was a problem hiding this comment. Updated to use a different signature for the credential based constructor - should likely help with both simplification/maintenance and user ex concerns. There was a problem hiding this comment. I'm just catching up here and I might have missed some things as changes have been made. I think the original comment was about the checks being done on I think @ramya-rao-a 's had 2 points:
The 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 (isConnectionStringConstructorInvoked) { | ||
| // connectionString and options based constructor was invoked | ||
| connectionString = hostOrConnectionString; | ||
| config = ConnectionConfig.create(connectionString); | ||
|
|
||
| options = credentialOrServiceBusClientOptions as ServiceBusClientOptions; | ||
| 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 { | ||
| // host, credential and options based constructor was invoked | ||
| if (!isTokenCredential(credentialOrServiceBusClientOptions)) { | ||
| throw new Error( | ||
| "'credentials' is a required parameter and must be an implementation of TokenCredential when using host based constructor overload." | ||
| ); | ||
| } | ||
| 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); | ||
| } | ||
|
|
@@ -153,33 +218,6 @@ export class ServiceBusClient { | |
| } | ||
| } | ||
|
|
||
| // /** | ||
| // * Creates a ServiceBusClient for the Service Bus Namespace represented by the given host using | ||
| // * the given TokenProvider. | ||
ramya-rao-a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to use latest of
core-amqpand fixing MessagingError as a consequence is being done as part of #6861Any reason to include these changes in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, ideally that should have been merged already - branch will get updated once that's done
EDIT: I think this was necessary for build to pass based on version used in event hubs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to use the
allowedAlternativeVersionssection in thecommon-versions.jsonfile to get rush to allow you to use different versions of@azure/core-amqpin Service Bus vs Event Hubs.This PR is for the Azure identity work and it would help if the changes here are restricted to using the
@azure/identitylibrary in the constructors.If #6861 gets merged before this PR, we can then
feature/service-bus-track-2common-versions.jsonfileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over the diffs - the duplicate changes related to core-amqp are very few and can be ignored - it's this one line change updating version, and the type casting ones in files
messageReceiver.ts,messageSender.tsandbatchingReceiver.ts. (You can ignore the files altogether if they already appear in #6861)All other 24-25 files are relevant and can be reviewed. The build succeeds as well.