Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CosmosClient: Add check for empty keys #1639

Merged
merged 2 commits into from
Jun 18, 2020
Merged

CosmosClient: Add check for empty keys #1639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7312a02
Adding checks
ealsur Jun 18, 2020
72fb339
tests
ealsur Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Microsoft.Azure.Cosmos/src/CosmosClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,12 +194,12 @@ public CosmosClient(
string authKeyOrResourceToken,
CosmosClientOptions clientOptions = null)
{
if (accountEndpoint == null)
if (string.IsNullOrEmpty(accountEndpoint))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do string.IsNullOrWhiteSpace?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the text is not empty, it gets checked after because it's used to create a Uri, which would fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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?

{
throw new ArgumentNullException(nameof(accountEndpoint));
}

if (authKeyOrResourceToken == null)
if (string.IsNullOrEmpty(authKeyOrResourceToken))
{
throw new ArgumentNullException(nameof(authKeyOrResourceToken));
}
Expand All @@ -222,12 +222,12 @@ internal CosmosClient(
CosmosClientOptions cosmosClientOptions,
DocumentClient documentClient)
{
if (accountEndpoint == null)
if (string.IsNullOrEmpty(accountEndpoint))
{
throw new ArgumentNullException(nameof(accountEndpoint));
}

if (authKeyOrResourceToken == null)
if (string.IsNullOrEmpty(authKeyOrResourceToken))
{
throw new ArgumentNullException(nameof(authKeyOrResourceToken));
}
Expand Down
4 changes: 2 additions & 2 deletions Microsoft.Azure.Cosmos/src/Fluent/CosmosClientBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ public CosmosClientBuilder(
string accountEndpoint,
string authKeyOrResourceToken)
{
if (accountEndpoint == null)
if (string.IsNullOrEmpty(accountEndpoint))
{
throw new ArgumentNullException(nameof(CosmosClientBuilder.accountEndpoint));
}

if (authKeyOrResourceToken == null)
if (string.IsNullOrEmpty(authKeyOrResourceToken))
{
throw new ArgumentNullException(nameof(authKeyOrResourceToken));
}
Expand Down
37 changes: 37 additions & 0 deletions Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/CosmosClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ namespace Microsoft.Azure.Cosmos.Tests
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.Azure.Cosmos.Fluent;
using Microsoft.VisualStudio.TestTools.UnitTesting;

[TestClass]
Expand Down Expand Up @@ -52,5 +53,41 @@ public async Task TestDispose()
catch (ObjectDisposedException) { }
}
}

[DataTestMethod]
[DataRow(null, "425Mcv8CXQqzRNCgFNjIhT424GK99CKJvASowTnq15Vt8LeahXTcN5wt3342vQ==")]
[DataRow(AccountEndpoint, null)]
[DataRow("", "425Mcv8CXQqzRNCgFNjIhT424GK99CKJvASowTnq15Vt8LeahXTcN5wt3342vQ==")]
[DataRow(AccountEndpoint, "")]
[ExpectedException(typeof(ArgumentNullException))]
public void InvalidEndpointAndKey(string endpoint, string key)
{
new CosmosClient(endpoint, key);
}

[DataTestMethod]
[DataRow(null, "425Mcv8CXQqzRNCgFNjIhT424GK99CKJvASowTnq15Vt8LeahXTcN5wt3342vQ==")]
[DataRow(AccountEndpoint, null)]
[DataRow("", "425Mcv8CXQqzRNCgFNjIhT424GK99CKJvASowTnq15Vt8LeahXTcN5wt3342vQ==")]
[DataRow(AccountEndpoint, "")]
[ExpectedException(typeof(ArgumentNullException))]
public void Builder_InvalidEndpointAndKey(string endpoint, string key)
{
new CosmosClientBuilder(endpoint, key);
}

[TestMethod]
public void InvalidConnectionString()
{
Assert.ThrowsException<ArgumentException>(() => new CosmosClient(""));
Assert.ThrowsException<ArgumentNullException>(() => new CosmosClient(null));
}

[TestMethod]
public void Builder_InvalidConnectionString()
{
Assert.ThrowsException<ArgumentException>(() => new CosmosClientBuilder(""));
Assert.ThrowsException<ArgumentNullException>(() => new CosmosClientBuilder(null));
}
}
}