-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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] ConnectionString parsing cleanup #10664
Conversation
sdk/servicebus/Azure.Messaging.ServiceBus/src/ServiceBusConnectionStringBuilder.cs
Show resolved
Hide resolved
Closing this PR for now, since it needs more discussion. |
@@ -115,7 +115,8 @@ public static ConnectionStringProperties Parse(string connectionString) | |||
{ | |||
parsedValues.EndpointToken = new UriBuilder(value) | |||
{ | |||
Scheme = ServiceBusEndpointScheme | |||
Scheme = ServiceBusEndpointScheme, | |||
Port = -1 |
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.
Hmm, how is this used?
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.
If I parse the below connection string without setting Port = -1
ConnectionStringParser.Parse("Endpoint=ns1.servicebus.windows.net/;SharedAccessKeyName=FakeKeyName;SharedAccessKey=FakeKey;")
It returns Endpoint - "sb://ns1.servicebus.windows.net:80/"
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.
Interesting, do we know why this wasn't needed in the EH version?
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.
If we parse this connection string without setting Port = -1
"Endpoint=sb://ns1.servicebus.windows.net/;SharedAccessKeyName=DummyKeyName;SharedAccessKey=DummyKey"
It returns the Endpoint as expected. Meaning if we pass the connection string endpoint starting with sb://
or pb://
or anything; It doesn't append the port and returns the Endpoint as expected.
I don't know about EH. I need to ask @jsquire.
sdk/servicebus/Azure.Messaging.ServiceBus/tests/Client/ServiceBusClientTests.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/tests/Client/ServiceBusClientTests.cs
Outdated
Show resolved
Hide resolved
sdk/servicebus/Azure.Messaging.ServiceBus/tests/Sender/SenderLiveTests.cs
Outdated
Show resolved
Hide resolved
new ServiceBusProcessor( | ||
CancellationToken cancellationToken = default) | ||
{ | ||
if (isConnectionPropertiesValidationNeeded) |
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.
Do we need this bool? I think we should be able to detect if we need to the validation based on whether Connection.ConnectionStringProperties is null.
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.
ConnectionStringProperties is a struct. It can never be null.
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.
Well we can check the EntityName property on it. If that is null, then no validation is needed.
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 we don't even need the ConnectionStringProperties property in ServiceBusConnection class. We can directly check if Connection.EntityPath is not null, then do the validation.
sdk/servicebus/Azure.Messaging.ServiceBus/src/ServiceBusClient.cs
Outdated
Show resolved
Hide resolved
@@ -250,12 +234,9 @@ public ServiceBusReceiver GetReceiver(string queueName) | |||
string topicName, | |||
string subscriptionName) | |||
{ | |||
if (isConnectionPropertiesValidationNeeded) | |||
if (!string.IsNullOrEmpty(Connection.EntityPath)) |
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.
super nit: move this check into ValidateEntityName.
#9884
connectionStringBuilder
class.