[EventHubs] Enable the FxCop Analysizers

As a follow up to Azure#11414, I was trying to understand why did not see errors from the "FxCop Analyiszers", which would have pointed out that issue to us. The reason was simple - we had disabled the analysis for these projects. I've re-enabled them, either fixing up issues or adding suppressions. We ended up catching some reasonable errors (a few cases where we called the ArgumentException constructor but messed up the order of the message and the parameter name) as well as a case where we were passing two parameters to an internal method but then disregarding them. In practice, the code we had was still "correct", because it happened to only be called from a single place today and the only caller was passing the same values that the implementation was actually using. Overall, I think taking this would be worthwhile. It brings this library back in line with the the others. I could imagine some library wide supressions of rules here, specifically the one about not calling virtual methods from constructors, since in practice we are unlikely to hit the problems the rule intends to guard for, but for now I've just made the diff look "as bad as possible" by doing supressions around the offending lines with justifications.
ellismg · Apr 21, 2020 · 58fc47a · 58fc47a
1 parent ffb3545
commit 58fc47a
Show file tree

Hide file tree

Showing 16 changed files with 68 additions and 26 deletions.
diff --git a/...enthub/Azure.Messaging.EventHubs.Processor/src/Azure.Messaging.EventHubs.Processor.csproj b/...enthub/Azure.Messaging.EventHubs.Processor/src/Azure.Messaging.EventHubs.Processor.csproj
@@ -4,7 +4,6 @@
     <Version>5.1.0-preview.2</Version>
     <PackageTags>Azure;Event Hubs;EventHubs;.NET;Event Processor;EventProcessor;$(PackageCommonTags)</PackageTags>
     <TargetFrameworks>$(RequiredTargetFrameworks)</TargetFrameworks>
-    <EnableFxCopAnalyzers>false</EnableFxCopAnalyzers>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs.Processor/src/EventProcessorClient.cs b/sdk/eventhub/Azure.Messaging.EventHubs.Processor/src/EventProcessorClient.cs
@@ -31,7 +31,9 @@ namespace Azure.Messaging.EventHubs
     ///   allowing the processor to be resilient in the face of errors.
     /// </summary>
     ///
+#pragma warning disable CA1001 // Types that own disposable fields should be disposable
     public class EventProcessorClient : EventProcessor<EventProcessorPartition>
+#pragma warning restore CA1001 // Types that own disposable fields should be disposable
     {
         /// <summary>The number of events to request as the maximum size for batches read from a partition.</summary>
         private const int ReadBatchSize = 15;
@@ -66,6 +68,7 @@ public class EventProcessorClient : EventProcessor<EventProcessorPartition>
         ///
         [SuppressMessage("Usage", "AZC0002:Ensure all service methods take an optional CancellationToken parameter.", Justification = "Guidance does not apply; this is an event.")]
         [SuppressMessage("Usage", "AZC0003:DO make service methods virtual.", Justification = "This member follows the standard .NET event pattern; override via the associated On<<EVENT>> method.")]
+        [SuppressMessage("Design", "CA1065:Do not raise exceptions in unexpected locations", Justification = "Guidelines do allow throwing NotSupportedException or ArgumentException here")]
         public event Func<PartitionInitializingEventArgs, Task> PartitionInitializingAsync
         {
             add
@@ -99,6 +102,7 @@ public event Func<PartitionInitializingEventArgs, Task> PartitionInitializingAsy
         ///
         [SuppressMessage("Usage", "AZC0002:Ensure all service methods take an optional CancellationToken parameter.", Justification = "Guidance does not apply; this is an event.")]
         [SuppressMessage("Usage", "AZC0003:DO make service methods virtual.", Justification = "This member follows the standard .NET event pattern; override via the associated On<<EVENT>> method.")]
+        [SuppressMessage("Design", "CA1065:Do not raise exceptions in unexpected locations", Justification = "Guidelines do allow throwing NotSupportedException or ArgumentException here")]
         public event Func<PartitionClosingEventArgs, Task> PartitionClosingAsync
         {
             add
@@ -133,6 +137,7 @@ public event Func<PartitionClosingEventArgs, Task> PartitionClosingAsync
         ///
         [SuppressMessage("Usage", "AZC0002:Ensure all service methods take an optional CancellationToken parameter.", Justification = "Guidance does not apply; this is an event.")]
         [SuppressMessage("Usage", "AZC0003:DO make service methods virtual.", Justification = "This member follows the standard .NET event pattern; override via the associated On<<EVENT>> method.")]
+        [SuppressMessage("Design", "CA1065:Do not raise exceptions in unexpected locations", Justification = "Guidelines do allow throwing NotSupportedException or ArgumentException here")]
         public event Func<ProcessEventArgs, Task> ProcessEventAsync
         {
             add
@@ -167,6 +172,7 @@ public event Func<ProcessEventArgs, Task> ProcessEventAsync
         ///
         [SuppressMessage("Usage", "AZC0002:Ensure all service methods take an optional CancellationToken parameter.", Justification = "Guidance does not apply; this is an event.")]
         [SuppressMessage("Usage", "AZC0003:DO make service methods virtual.", Justification = "This member follows the standard .NET event pattern; override via the associated On<<EVENT>> method.")]
+        [SuppressMessage("Design", "CA1065:Do not raise exceptions in unexpected locations", Justification = "Guidelines do allow throwing NotSupportedException or ArgumentException here")]
         public event Func<ProcessErrorEventArgs, Task> ProcessErrorAsync
         {
             add

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs.Processor/src/Storage/BlobsCheckpointStore.cs b/sdk/eventhub/Azure.Messaging.EventHubs.Processor/src/Storage/BlobsCheckpointStore.cs
@@ -99,7 +99,7 @@ public override async Task<IEnumerable<EventProcessorPartitionOwnership>> ListOw
 
             try
             {
-                var prefix = string.Format(OwnershipPrefix, fullyQualifiedNamespace.ToLowerInvariant(), eventHubName.ToLowerInvariant(), consumerGroup.ToLowerInvariant());
+                var prefix = string.Format(CultureInfo.InvariantCulture, OwnershipPrefix, fullyQualifiedNamespace.ToLowerInvariant(), eventHubName.ToLowerInvariant(), consumerGroup.ToLowerInvariant());
 
                 Func<CancellationToken, Task<List<EventProcessorPartitionOwnership>>> listOwnershipAsync = async listOwnershipToken =>
                 {
@@ -168,7 +168,7 @@ public override async Task<IEnumerable<EventProcessorPartitionOwnership>> ClaimO
 
                 var blobRequestConditions = new BlobRequestConditions();
 
-                var blobName = string.Format(OwnershipPrefix + ownership.PartitionId, ownership.FullyQualifiedNamespace.ToLowerInvariant(), ownership.EventHubName.ToLowerInvariant(), ownership.ConsumerGroup.ToLowerInvariant());
+                var blobName = string.Format(CultureInfo.InvariantCulture, OwnershipPrefix + ownership.PartitionId, ownership.FullyQualifiedNamespace.ToLowerInvariant(), ownership.EventHubName.ToLowerInvariant(), ownership.ConsumerGroup.ToLowerInvariant());
                 var blobClient = ContainerClient.GetBlobClient(blobName);
 
                 try
@@ -277,7 +277,7 @@ public override async Task<IEnumerable<EventProcessorCheckpoint>> ListCheckpoint
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
             Logger.ListCheckpointsStart(fullyQualifiedNamespace, eventHubName, consumerGroup);
 
-            var prefix = string.Format(CheckpointPrefix, fullyQualifiedNamespace.ToLowerInvariant(), eventHubName.ToLowerInvariant(), consumerGroup.ToLowerInvariant());
+            var prefix = string.Format(CultureInfo.InvariantCulture, CheckpointPrefix, fullyQualifiedNamespace.ToLowerInvariant(), eventHubName.ToLowerInvariant(), consumerGroup.ToLowerInvariant());
             var checkpointCount = 0;
 
             Func<CancellationToken, Task<IEnumerable<EventProcessorCheckpoint>>> listCheckpointsAsync = async listCheckpointsToken =>
@@ -359,7 +359,7 @@ public override async Task UpdateCheckpointAsync(EventProcessorCheckpoint checkp
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
             Logger.UpdateCheckpointStart(checkpoint.PartitionId, checkpoint.FullyQualifiedNamespace, checkpoint.EventHubName, checkpoint.ConsumerGroup);
 
-            var blobName = string.Format(CheckpointPrefix + checkpoint.PartitionId, checkpoint.FullyQualifiedNamespace.ToLowerInvariant(), checkpoint.EventHubName.ToLowerInvariant(), checkpoint.ConsumerGroup.ToLowerInvariant());
+            var blobName = string.Format(CultureInfo.InvariantCulture, CheckpointPrefix + checkpoint.PartitionId, checkpoint.FullyQualifiedNamespace.ToLowerInvariant(), checkpoint.EventHubName.ToLowerInvariant(), checkpoint.ConsumerGroup.ToLowerInvariant());
             var blobClient = ContainerClient.GetBlobClient(blobName);
 
             var metadata = new Dictionary<string, string>()

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpClient.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpClient.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Diagnostics;
+using System.Globalization;
 using System.Security.Authentication;
 using System.Threading;
 using System.Threading.Tasks;
@@ -462,7 +463,7 @@ public override async Task CloseAsync(CancellationToken cancellationToken)
 
             _closed = true;
 
-            var clientId = GetHashCode().ToString();
+            var clientId = GetHashCode().ToString(CultureInfo.InvariantCulture);
             var clientType = GetType();
 
             try

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConnectionScope.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConnectionScope.cs
@@ -176,9 +176,12 @@ public AmqpConnectionScope(Uri serviceEndpoint,
             Transport = transport;
             Proxy = proxy;
             TokenProvider = new CbsTokenProvider(new EventHubTokenCredential(credential, serviceEndpoint.ToString()), OperationCancellationSource.Token);
-            Id = identifier ?? $"{ eventHubName }-{ Guid.NewGuid().ToString("D").Substring(0, 8) }";
+            Id = identifier ?? $"{ eventHubName }-{ Guid.NewGuid().ToString("D", CultureInfo.InvariantCulture).Substring(0, 8) }";
 
+#pragma warning disable CA2214 // Do not call overridable methods in constructors. This internal method is virtual for testing purposes.
             Task<AmqpConnection> connectionFactory(TimeSpan timeout) => CreateAndOpenConnectionAsync(AmqpVersion, ServiceEndpoint, Transport, Proxy, Id, timeout);
+#pragma warning restore CA2214 // Do not call overridable methods in constructors
+
             ActiveConnection = new FaultTolerantAmqpObject<AmqpConnection>(connectionFactory, CloseConnection);
         }
 
@@ -253,7 +256,7 @@ public virtual async Task<ReceivingAmqpLink> OpenConsumerLinkAsync(string consum
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
 
             var stopWatch = Stopwatch.StartNew();
-            var consumerEndpoint = new Uri(ServiceEndpoint, string.Format(ConsumerPathSuffixMask, EventHubName, consumerGroup, partitionId));
+            var consumerEndpoint = new Uri(ServiceEndpoint, string.Format(CultureInfo.InvariantCulture, ConsumerPathSuffixMask, EventHubName, consumerGroup, partitionId));
 
             var connection = await ActiveConnection.GetOrCreateAsync(timeout).ConfigureAwait(false);
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
@@ -295,7 +298,7 @@ public virtual async Task<SendingAmqpLink> OpenProducerLinkAsync(string partitio
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
 
             var stopWatch = Stopwatch.StartNew();
-            var path = (string.IsNullOrEmpty(partitionId)) ? EventHubName : string.Format(PartitionProducerPathSuffixMask, EventHubName, partitionId);
+            var path = (string.IsNullOrEmpty(partitionId)) ? EventHubName : string.Format(CultureInfo.InvariantCulture, PartitionProducerPathSuffixMask, EventHubName, partitionId);
             var producerEndpoint = new Uri(ServiceEndpoint, path);
 
             var connection = await ActiveConnection.GetOrCreateAsync(timeout).ConfigureAwait(false);
@@ -371,10 +374,11 @@ protected virtual async Task<AmqpConnection> CreateAndOpenConnectionAsync(Versio
 
             stopWatch.Stop();
 
+#pragma warning disable CA1806 // Do not ignore method results
             // Create the CBS link that will be used for authorization.  The act of creating the link will associate
             // it with the connection.
-
             new AmqpCbsLink(connection);
+#pragma warning restore CA1806 // Do not ignore method results
 
             // When the connection is closed, close each of the links associated with it.
 
@@ -951,7 +955,7 @@ private static void ValidateTransport(EventHubsTransportType transport)
         {
             if ((transport != EventHubsTransportType.AmqpTcp) && (transport != EventHubsTransportType.AmqpWebSockets))
             {
-                throw new ArgumentException(nameof(transport), string.Format(CultureInfo.CurrentCulture, Resources.UnknownConnectionType, transport));
+                throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Resources.UnknownConnectionType, transport), nameof(transport));
             }
         }
     }

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConsumer.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConsumer.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Globalization;
 using System.Runtime.ExceptionServices;
 using System.Threading;
 using System.Threading.Tasks;
@@ -341,7 +342,7 @@ public override async Task CloseAsync(CancellationToken cancellationToken)
 
             _closed = true;
 
-            var clientId = GetHashCode().ToString();
+            var clientId = GetHashCode().ToString(CultureInfo.InvariantCulture);
             var clientType = GetType();
 
             try
@@ -401,12 +402,12 @@ private async Task<ReceivingAmqpLink> CreateConsumerLinkAsync(string consumerGro
                 link = await ConnectionScope.OpenConsumerLinkAsync(
                     consumerGroup,
                     partitionId,
-                    CurrentEventPosition,
+                    eventStartingPosition,
                     timeout,
                     prefetchCount,
                     ownerLevel,
                     trackLastEnqueuedEventProperties,
-                    CancellationToken.None).ConfigureAwait(false);
+                    cancellationToken).ConfigureAwait(false);
             }
             catch (InvalidOperationException ex)
             {

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpProducer.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpProducer.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Globalization;
 using System.Runtime.ExceptionServices;
 using System.Threading;
 using System.Threading.Tasks;
@@ -279,7 +280,7 @@ public override async Task CloseAsync(CancellationToken cancellationToken)
 
             _closed = true;
 
-            var clientId = GetHashCode().ToString();
+            var clientId = GetHashCode().ToString(CultureInfo.InvariantCulture);
             var clientType = GetType();
 
             try
@@ -338,7 +339,7 @@ protected virtual async Task SendAsync(Func<AmqpMessage> messageFactory,
                     try
                     {
                         using AmqpMessage batchMessage = messageFactory();
-                        messageHash = batchMessage.GetHashCode().ToString();
+                        messageHash = batchMessage.GetHashCode().ToString(CultureInfo.InvariantCulture);
 
                         // Creation of the link happens without explicit knowledge of the cancellation token
                         // used for this operation; validate the token state before attempting link creation and
@@ -355,7 +356,7 @@ protected virtual async Task SendAsync(Func<AmqpMessage> messageFactory,
 
                         if (batchMessage.SerializedMessageSize > MaximumMessageSize)
                         {
-                            throw new EventHubsException(EventHubName, string.Format(Resources.MessageSizeExceeded, messageHash, batchMessage.SerializedMessageSize, MaximumMessageSize), EventHubsException.FailureReason.MessageSizeExceeded);
+                            throw new EventHubsException(EventHubName, string.Format(CultureInfo.CurrentCulture, Resources.MessageSizeExceeded, messageHash, batchMessage.SerializedMessageSize, MaximumMessageSize), EventHubsException.FailureReason.MessageSizeExceeded);
                         }
 
                         // Attempt to send the message batch.

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Azure.Messaging.EventHubs.csproj b/sdk/eventhub/Azure.Messaging.EventHubs/src/Azure.Messaging.EventHubs.csproj
@@ -4,7 +4,6 @@
     <Version>5.1.0-preview.2</Version>
     <PackageTags>Azure;Event Hubs;EventHubs;.NET;AMQP;IoT;$(PackageCommonTags)</PackageTags>
     <TargetFrameworks>$(RequiredTargetFrameworks)</TargetFrameworks>
-    <EnableFxCopAnalyzers>false</EnableFxCopAnalyzers>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Consumer/EventHubConsumerClient.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Consumer/EventHubConsumerClient.cs
@@ -601,7 +601,7 @@ public virtual async Task CloseAsync(CancellationToken cancellationToken = defau
 
             IsClosed = true;
 
-            var clientHash = GetHashCode().ToString();
+            var clientHash = GetHashCode().ToString(CultureInfo.InvariantCulture);
             EventHubsEventSource.Log.ClientCloseStart(typeof(EventHubConsumerClient), EventHubName, clientHash);
 
             // Attempt to close the active transport consumers.  In the event that an exception is encountered,

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Consumer/EventPosition.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Consumer/EventPosition.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.ComponentModel;
+using System.Globalization;
 using Azure.Core;
 
 namespace Azure.Messaging.EventHubs.Consumer
@@ -85,7 +86,7 @@ public struct EventPosition : IEquatable<EventPosition>
         /// <returns>The position of the specified event.</returns>
         ///
         public static EventPosition FromOffset(long offset,
-                                               bool isInclusive = true) => FromOffset(offset.ToString(), isInclusive);
+                                               bool isInclusive = true) => FromOffset(offset.ToString(CultureInfo.InvariantCulture), isInclusive);
 
         /// <summary>
         ///   Corresponds to the event in the partition having a specified sequence number associated with it.

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/Core/DeveloperCodeException.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/Core/DeveloperCodeException.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 
 namespace Azure.Messaging.EventHubs
 {
@@ -13,6 +14,7 @@ namespace Azure.Messaging.EventHubs
     ///
     /// <seealso cref="System.Exception" />
     ///
+    [SuppressMessage("Design", "CA1064:Exceptions should be public", Justification = "This exception is not visible to user code")]
     internal class DeveloperCodeException : Exception
     {
         /// <summary>

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/EventHubConnection.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/EventHubConnection.cs
@@ -172,7 +172,10 @@ public EventHubConnection(string connectionString,
             FullyQualifiedNamespace = fullyQualifiedNamespace;
             EventHubName = eventHubName;
             Options = connectionOptions;
+
+#pragma warning disable CA2214 // Do not call overridable methods in constructors. This internal method is virtual for testing purposes.
             InnerClient = CreateTransportClient(fullyQualifiedNamespace, eventHubName, tokenCredentials, connectionOptions);
+#pragma warning restore CA2214 // Do not call overridable methods in constructors.
         }
 
         /// <summary>
@@ -212,7 +215,9 @@ public EventHubConnection(string fullyQualifiedNamespace,
             EventHubName = eventHubName;
             Options = connectionOptions;
 
+#pragma warning disable CA2214 // Do not call overridable methods in constructors. This internal method is virtual for testing purposes.
             InnerClient = CreateTransportClient(fullyQualifiedNamespace, eventHubName, tokenCredential, connectionOptions);
+#pragma warning restore CA2214 // Do not call overridable methods in constructors.
         }
 
         /// <summary>
@@ -427,7 +432,9 @@ internal virtual TransportClient CreateTransportClient(string fullyQualifiedName
                     return new AmqpClient(fullyQualifiedNamespace, eventHubName, credential, options);
 
                 default:
+#pragma warning disable CA2208 // Instantiate argument exceptions correctly.  "TransportType" is a reasonable name.  It's the property on the options argument which is invalid.
                     throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, Resources.InvalidTransportType, options.TransportType.ToString()), nameof(options.TransportType));
+#pragma warning restore CA2208 // Instantiate argument exceptions correctly
             }
         }
 
@@ -455,7 +462,7 @@ private static string BuildAudienceResource(EventHubsTransportType transportType
                 UserName = string.Empty,
             };
 
-            if (builder.Path.EndsWith("/"))
+            if (builder.Path.EndsWith("/", StringComparison.Ordinal))
             {
                 builder.Path = builder.Path.TrimEnd('/');
             }

diff --git a/sdk/eventhub/Azure.Messaging.EventHubs/src/EventHubsRetryOptions.cs b/sdk/eventhub/Azure.Messaging.EventHubs/src/EventHubsRetryOptions.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Diagnostics.CodeAnalysis;
 using Azure.Core;
 
 namespace Azure.Messaging.EventHubs
@@ -83,6 +84,7 @@ public TimeSpan MaximumDelay
         ///   attempt or a retry.
         /// </summary>
         ///
+        [SuppressMessage("Usage", "CA2208:Instantiate argument exceptions correctly", Justification = "We believe using the property name instead of 'value' is more intuitive")]
         public TimeSpan TryTimeout
         {
             get => _tryTimeout;