[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.
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.
Interesting, do we know why this wasn't needed in the EH version?
There was a problem hiding this comment.
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.
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.
ConnectionStringProperties is a struct. It can never be null.
There was a problem hiding this comment.
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.
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.
super nit: move this check into ValidateEntityName.
#9884
connectionStringBuilderclass.