From 2911abee3b0b322e71d61f282bb67c8ca8ec288c Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 5 May 2016 15:25:25 -0700 Subject: [PATCH] Refactored MultiShardConnection constructor input validation and avoid multiply evaluating enumerables. Added unit tests verifying that enumerables are only evaluated once. Fixes #76. --- .../Query/MultiShardConnection.cs | 53 ++++++++------ .../ElasticScale.Query.UnitTests.csproj | 6 +- .../EnumerableHelpers.cs | 49 +++++++++++++ .../MultiShardConnectionTests.cs | 71 +++++++++++++++++++ 4 files changed, 155 insertions(+), 24 deletions(-) create mode 100644 Test/ElasticScale.Query.UnitTests/EnumerableHelpers.cs create mode 100644 Test/ElasticScale.Query.UnitTests/MultiShardConnectionTests.cs diff --git a/Src/ElasticScale.Client/Query/MultiShardConnection.cs b/Src/ElasticScale.Client/Query/MultiShardConnection.cs index c6d8cdd..680ac50 100644 --- a/Src/ElasticScale.Client/Query/MultiShardConnection.cs +++ b/Src/ElasticScale.Client/Query/MultiShardConnection.cs @@ -66,19 +66,24 @@ public MultiShardConnection(IEnumerable shards, string connectionString) { throw new ArgumentNullException("connectionString"); } + if (shards == null) + { + throw new ArgumentNullException("shards"); + } // Enhance the ApplicationName with this library's name as a suffix // Devnote: If connection string specifies Active Directory authentication and runtime is not // .NET 4.6 or higher, then below call will throw. SqlConnectionStringBuilder connectionStringBuilder = new SqlConnectionStringBuilder( - connectionString).WithApplicationNameSuffix(ApplicationNameSuffix); + connectionString).WithApplicationNameSuffix(ApplicationNameSuffix); + ValidateConnectionString(connectionStringBuilder); - ValidateConnectionArguments(shards, "shards", connectionStringBuilder); + // Force evaluation of the input enumerable so that we don't evaluate it multiple times later + this.Shards = shards.ToList(); + ValidateNotEmpty(this.Shards, "shards"); - this.Shards = shards; - this.ShardConnections = shards.Select( - s => (CreateDbConnectionForLocation(s.Location, connectionStringBuilder)) - ).ToList(); + this.ShardConnections = this.Shards.Select( + s => CreateDbConnectionForLocation(s.Location, connectionStringBuilder)).ToList(); } /// @@ -99,21 +104,27 @@ public MultiShardConnection(IEnumerable shardLocations, string co { throw new ArgumentNullException("connectionString"); } + if (shardLocations == null) + { + throw new ArgumentNullException("shardLocations"); + } // Enhance the ApplicationName with this library's name as a suffix // Devnote: If connection string specifies Active Directory authentication and runtime is not // .NET 4.6 or higher, then below call will throw. SqlConnectionStringBuilder connectionStringBuilder = new SqlConnectionStringBuilder( - connectionString).WithApplicationNameSuffix(ApplicationNameSuffix); + connectionString).WithApplicationNameSuffix(ApplicationNameSuffix); + ValidateConnectionString(connectionStringBuilder); - ValidateConnectionArguments(shardLocations, "shardLocations", connectionStringBuilder); + // Force evaluation of the input enumerable so that we don't evaluate it multiple times later + IList shardLocationsList = shardLocations.ToList(); + ValidateNotEmpty(shardLocationsList, "shardLocations"); this.Shards = null; - this.ShardConnections = shardLocations.Select( - s => (CreateDbConnectionForLocation(s, connectionStringBuilder)) - ).ToList(); + this.ShardConnections = shardLocationsList.Select( + s => CreateDbConnectionForLocation(s, connectionStringBuilder)).ToList(); } - + /// /// Creates an instance of this class /// /* TEST ONLY */ @@ -196,21 +207,19 @@ public void Dispose() #region Helpers - private static void ValidateConnectionArguments( + private static void ValidateNotEmpty( IEnumerable namedCollection, - string collectionName, - SqlConnectionStringBuilder connectionStringBuilder) + string collectionName) { - if (namedCollection == null) - { - throw new ArgumentNullException(collectionName); - } - - if (0 == namedCollection.Count()) + if (!namedCollection.Any()) { throw new ArgumentException(string.Format("No {0} provided.", collectionName)); } + } + private static void ValidateConnectionString( + SqlConnectionStringBuilder connectionStringBuilder) + { // Datasource must not be set if (!string.IsNullOrEmpty(connectionStringBuilder.DataSource)) { @@ -228,7 +237,7 @@ private static void ValidateConnectionArguments( // [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")] private static Tuple CreateDbConnectionForLocation( - ShardLocation shardLocation, + ShardLocation shardLocation, SqlConnectionStringBuilder connectionStringBuilder) { return new Tuple diff --git a/Test/ElasticScale.Query.UnitTests/ElasticScale.Query.UnitTests.csproj b/Test/ElasticScale.Query.UnitTests/ElasticScale.Query.UnitTests.csproj index 205ea6b..03d701c 100644 --- a/Test/ElasticScale.Query.UnitTests/ElasticScale.Query.UnitTests.csproj +++ b/Test/ElasticScale.Query.UnitTests/ElasticScale.Query.UnitTests.csproj @@ -1,4 +1,4 @@ - + {56DDEC7E-0B14-4636-B287-DB38DEC55192} @@ -49,6 +49,8 @@ + + Component @@ -112,4 +114,4 @@ --> - + \ No newline at end of file diff --git a/Test/ElasticScale.Query.UnitTests/EnumerableHelpers.cs b/Test/ElasticScale.Query.UnitTests/EnumerableHelpers.cs new file mode 100644 index 0000000..28f5ad4 --- /dev/null +++ b/Test/ElasticScale.Query.UnitTests/EnumerableHelpers.cs @@ -0,0 +1,49 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Threading; + +namespace Microsoft.Azure.SqlDatabase.ElasticScale.Query.UnitTests +{ + public static class EnumerableHelpers + { + public static IEnumerable ToConsumable(this IEnumerable source) + { + return new ConsumingEnumerable(source); + } + + /// + /// IEnumerable wrapper that can only be enumerated once. + /// + /// + public class ConsumingEnumerable : IEnumerable + { + private IEnumerable _source; + private int _consumed; + + public ConsumingEnumerable(IEnumerable source) + { + _source = source; + _consumed = 0; + } + + public IEnumerator GetEnumerator() + { + int wasConsumed = Interlocked.Exchange(ref _consumed, 1); + if (wasConsumed == 0) + { + return _source.GetEnumerator(); + } + else + { + throw new InvalidOperationException("GetEnumerator() has already been called. Cannot enumerate more than once"); + } + } + + IEnumerator IEnumerable.GetEnumerator() + { + return GetEnumerator(); + } + } + } +} diff --git a/Test/ElasticScale.Query.UnitTests/MultiShardConnectionTests.cs b/Test/ElasticScale.Query.UnitTests/MultiShardConnectionTests.cs new file mode 100644 index 0000000..c43b6ed --- /dev/null +++ b/Test/ElasticScale.Query.UnitTests/MultiShardConnectionTests.cs @@ -0,0 +1,71 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.Azure.SqlDatabase.ElasticScale.ShardManagement; +using Microsoft.Azure.SqlDatabase.ElasticScale.Test.Common; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.Azure.SqlDatabase.ElasticScale.Query.UnitTests +{ + [TestClass] + public class MultiShardConnectionTests + { + private const string dummyConnectionString = "User ID=x;Password=x"; + + /// + /// Verifies that + /// throws when the input shardlocations are null + /// + [TestMethod] + public void TestMultiShardConnectionConstructorThrowsNullShardLocations() + { + AssertExtensions.AssertThrows( + () => new MultiShardConnection((IEnumerable)null, dummyConnectionString)); + } + + /// + /// Verifies that + /// throws when the input shards are null + /// + [TestMethod] + public void TestMultiShardConnectionConstructorThrowsNullShards() + { + AssertExtensions.AssertThrows( + () => new MultiShardConnection((IEnumerable)null, dummyConnectionString)); + } + + /// + /// Verifies that + /// does not multiply evaluate the input enumerable + /// + [TestMethod] + public void TestMultiShardConnectionConstructorEvaluatesShardLocations() + { + List shardLocations = new List + { + new ShardLocation("server1", "db1"), + new ShardLocation("server2", "db2"), + new ShardLocation("server3", "db3") + }; + + MultiShardConnection conn = new MultiShardConnection(shardLocations.ToConsumable(), dummyConnectionString); + AssertExtensions.AssertSequenceEqual(shardLocations, conn.ShardLocations); + } + + /// + /// Verifies that + /// does not multiply evaluate the input enumerable + /// + [TestMethod] + public void TestMultiShardConnectionConstructorEvaluatesShards() + { + MultiShardTestUtils.DropAndCreateDatabases(); + ShardMap shardMap = MultiShardTestUtils.CreateAndGetTestShardMap(); + + List shards = shardMap.GetShards().ToList(); + + MultiShardConnection conn = new MultiShardConnection(shards.ToConsumable(), dummyConnectionString); + AssertExtensions.AssertSequenceEqual(shards, conn.Shards); + } + } +}