-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ramya0820
merged 25 commits into
Azure:feature/service-bus-track-2
from
ramya0820:issue-4262
Jan 15, 2020
Merged
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
2d4f970
Update constructor
fa13083
Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…
b7c5198
Update api.md file
afc8689
Replicate Event Hubs client creation
3bcb540
Clean up
99d2d57
Fix setting of options
2b00a70
Update docs
1c8760a
Handle undefined input correctly
9883ac9
Update api.md file
ced6b23
Attempt adding tests
e164436
Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…
8d8abbe
Fix tests
2a7868e
Address comments
22c5103
Clean up
115b53f
Update signature
9d0597b
Enable streaming receiver test
865b0d8
Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…
c4a2267
Remove unused util
33ce753
Clean up comments
a749abd
Switch parameters
d5b3eb0
Remove need for aadUtils.browser
51af115
Clean up README file for tests
d0dcec0
Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…
e05a944
Revert "Clean up README file for tests"
e9d9dfb
Address comments
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The above checks are not needed.
ConnectionConfig.create()
ensures thatString()
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 fromConnectionConfig.create()
.I would suggest the below for handling the overloads
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.
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.
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 tostring
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.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.
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 usingstring
where possible.And yes, the conversion was to help with both compile and run time scenarios.
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.
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 downstreamConnectionConfig.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.
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.
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.
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.)
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.
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.
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
andhost
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
.I think @ramya-rao-a 's had 2 points:
hostOrConnectionString
is a string by wrapping it inString
: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).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#L339Event 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!