[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 |
|---|---|---|
|
|
@@ -372,8 +372,9 @@ export interface ServiceBusAtomManagementClientOptions { | |
|
|
||
| // @public | ||
| export class ServiceBusClient { | ||
| constructor(connectionString: string, options?: ServiceBusClientOptions); | ||
| constructor(credential: TokenCredential, host: string, options?: ServiceBusClientOptions); | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. Oh, could you point to which guideline is being considered for this? Because I found the following that seems similar / applicable for There was a problem hiding this comment. Link was hidden in the word 'guidelines' :) See the Client API example.
There was a problem hiding this comment. 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) There was a problem hiding this comment. Unable to reply to other comment for some reason :? 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). There was a problem hiding this comment. 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. There was a problem hiding this comment. To summarize the thread here - the user experience for when credential is passed in as 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. 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? |
||
| close(): Promise<any>; | ||
| createQueueClient(queueName: string): QueueClient; | ||
| createSubscriptionClient(topicName: string, subscriptionName: string): SubscriptionClient; | ||
| createTopicClient(topicName: string): TopicClient; | ||
|
|
||
| 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,78 @@ 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 credential - credential that implements the TokenCredential interface. | ||
| * @param host - The host name for the Service Bus namespace. This is likely to be similar to | ||
| * <yournamespace>.servicebus.windows.net | ||
| * @param options - Options to control ways to interact with the Service Bus | ||
| * Namespace. | ||
| */ | ||
| constructor(credential: TokenCredential, host: string, options?: ServiceBusClientOptions); | ||
|
|
||
| constructor( | ||
| connectionStringOrCredential: string | TokenCredential, | ||
| hostOrServiceBusClientOptions?: string | ServiceBusClientOptions, | ||
| options?: ServiceBusClientOptions | ||
| ) { | ||
| let config; | ||
| let credential; | ||
| let connectionString; | ||
|
|
||
| if (connectionStringOrCredential == undefined) { | ||
| throw new Error("Input parameter 'connectionString' or 'credentials' must be defined."); | ||
| } | ||
|
|
||
| if (!options) { | ||
| options = {}; | ||
| } | ||
|
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 (typeof connectionStringOrCredential == "string") { | ||
| // connectionString and options based constructor was invoked | ||
| connectionString = connectionStringOrCredential; | ||
| config = ConnectionConfig.create(connectionString); | ||
|
|
||
| options = hostOrServiceBusClientOptions 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 { | ||
| // credential, host and options based constructor was invoked | ||
| if (!isTokenCredential(connectionStringOrCredential)) { | ||
| throw new Error( | ||
| "'credentials' is a required parameter and must be an implementation of TokenCredential when using host based constructor overload." | ||
| ); | ||
| } | ||
| credential = connectionStringOrCredential as TokenCredential; | ||
|
|
||
| hostOrServiceBusClientOptions = String(hostOrServiceBusClientOptions); | ||
|
|
||
| if (!hostOrServiceBusClientOptions.endsWith("/")) { | ||
| hostOrServiceBusClientOptions += "/"; | ||
| } | ||
| connectionString = `Endpoint=sb://${hostOrServiceBusClientOptions};SharedAccessKeyName=defaultKeyName;SharedAccessKey=defaultKeyValue;`; | ||
| config = ConnectionConfig.create(connectionString); | ||
| } | ||
|
|
||
| this.name = config.endpoint; | ||
| this._context = ConnectionContext.create(config, credential, options); | ||
| } | ||
|
|
@@ -152,95 +208,4 @@ export class ServiceBusClient { | |
| throw errObj; | ||
| } | ||
| } | ||
| } | ||
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.