CosmosClient: Add check for empty keys #1639
Conversation
ealsur
requested review from
bchong95,
FabianMeiswinkel,
j82w,
khdang,
kirankumarkolli,
kirillg and
sboshra
as code owners
| @@ -194,12 +194,12 @@ protected CosmosClient() | |||
| string authKeyOrResourceToken, | |||
| CosmosClientOptions clientOptions = null) | |||
| { | |||
| if (accountEndpoint == null) | |||
| if (string.IsNullOrEmpty(accountEndpoint)) | |||
There was a problem hiding this comment.
Should we do string.IsNullOrWhiteSpace?
There was a problem hiding this comment.
If the text is not empty, it gets checked after because it's used to create a Uri, which would fail
There was a problem hiding this comment.
+1 on String.IsNullOrWhietspace for consistency - better to throw here than depend on Uri
Technically I think this can be considered a breaking change - we change the exception type in these cases. I can't think of any use case where customers would actually be broken for scenarios that work today (even when doing some custom error handling on the 401 etc.) - so I definitely don't mean this as a blocking comment - but I would call it out very explicitly in the changelog - does that make sense?
* Adding checks * tests
|
Closing due to in-activity, pease feel free to re-open. |
Description
Currently we check for
nullparameters on theCosmosClientandCosmosClientBuilderconstructor. But if customers are reading the key (for example) from a configuration source (like KeyVault) where the value stored there is an empty string, this leads to a support case for a 401 - Not Authorized.401 investigations are not easy, and from past experience, this is an easy fix that can rule out one of the most common scenarios.
Type of change