Update arg validation (Azure#16885)

* Update arg validation * Update validation for AMQP models * API updates * Perf enhancements * PR fb
AzureSDKAutomation · Nov 12, 2020 · ed8416f · ed8416f
1 parent 0a025d3
commit ed8416f
Show file tree

Hide file tree

Showing 23 changed files with 199 additions and 86 deletions.
diff --git a/sdk/core/Azure.Core.Amqp/src/AmqpAddress.cs b/sdk/core/Azure.Core.Amqp/src/AmqpAddress.cs
@@ -22,6 +22,7 @@ public struct AmqpAddress : IEquatable<AmqpAddress>
         /// <param name="address">The address.</param>
         public AmqpAddress(string address)
         {
+            Argument.AssertNotNull(address, nameof(address));
             _address = address;
         }
 

diff --git a/sdk/core/Azure.Core.Amqp/src/AmqpMessageBody.cs b/sdk/core/Azure.Core.Amqp/src/AmqpMessageBody.cs
@@ -30,7 +30,8 @@ public class AmqpMessageBody
         /// <param name="data">The data sections.</param>
         public AmqpMessageBody(IEnumerable<ReadOnlyMemory<byte>> data)
         {
-            _data = data ?? Enumerable.Empty<ReadOnlyMemory<byte>>();
+            Argument.AssertNotNull(data, nameof(data));
+            _data = data;
             BodyType = AmqpMessageBodyType.Data;
         }
 

diff --git a/sdk/core/Azure.Core.Amqp/src/AmqpMessageId.cs b/sdk/core/Azure.Core.Amqp/src/AmqpMessageId.cs
@@ -12,7 +12,7 @@ namespace Azure.Core.Amqp
     /// </summary>
     public struct AmqpMessageId : IEquatable<AmqpMessageId>
     {
-        private readonly string? _messageIdString;
+        private readonly string _messageIdString;
 
         /// <summary>
         /// Initializes a new <see cref="AmqpMessageId"/> using the provided
@@ -22,6 +22,7 @@ public struct AmqpMessageId : IEquatable<AmqpMessageId>
         /// <param name="messageId">The message Id.</param>
         public AmqpMessageId(string messageId)
         {
+            Argument.AssertNotNull(messageId, nameof(messageId));
             _messageIdString = messageId;
         }
 
@@ -30,7 +31,7 @@ public AmqpMessageId(string messageId)
         /// </summary>
         ///
         /// <returns>A <see cref="string"/> from the value of this instance.</returns>
-        public override string ToString() => _messageIdString!;
+        public override string ToString() => _messageIdString;
 
         /// <summary>
         /// Determines whether the provided object is equal to the current object.
@@ -69,7 +70,7 @@ public override int GetHashCode()
         /// <see cref="AmqpMessageId"/>; otherwise, <see langword="false" />.
         /// </returns>
         public bool Equals(AmqpMessageId other) =>
-            other.Equals(_messageIdString!);
+            other.Equals(_messageIdString);
 
         /// <summary>
         /// Determines whether the provided <see cref="string"/> is equal to the current instance.

diff --git a/sdk/core/Azure.Core.Amqp/tests/AmqpAddressTests.cs b/sdk/core/Azure.Core.Amqp/tests/AmqpAddressTests.cs
@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System;
 using NUnit.Framework;
 
 namespace Azure.Core.Amqp.Tests
@@ -17,5 +18,13 @@ public void CanCreateFromString()
             Assert.True(address.Equals((object)new AmqpAddress("address")));
             Assert.False(address.Equals(new AmqpMessageId("messageId2")));
         }
+
+        [Test]
+        public void CannotCreateFromNullAddress()
+        {
+            Assert.That(
+                () => new AmqpAddress(null),
+                Throws.InstanceOf<ArgumentNullException>());
+        }
     }
 }
diff --git a/sdk/core/Azure.Core.Amqp/tests/AmqpDataMessageBodyTests.cs b/sdk/core/Azure.Core.Amqp/tests/AmqpDataMessageBodyTests.cs
@@ -2,7 +2,6 @@
 // Licensed under the MIT License.
 
 using System;
-using System.Collections.Generic;
 using NUnit.Framework;
 
 namespace Azure.Core.Amqp.Tests
@@ -16,11 +15,14 @@ public void CanCreateDataBody()
             Assert.AreEqual(AmqpMessageBodyType.Data, body.BodyType);
             Assert.IsTrue(body.TryGetData(out var data));
             Assert.NotNull(data);
+        }
 
-            body = new AmqpMessageBody(null);
-            Assert.AreEqual(AmqpMessageBodyType.Data, body.BodyType);
-            Assert.IsTrue(body.TryGetData(out data));
-            Assert.NotNull(data);
+        [Test]
+        public void CannotCreateFromNullBody()
+        {
+            Assert.That(
+                () => new AmqpMessageBody(null),
+                Throws.InstanceOf<ArgumentNullException>());
         }
     }
 }
diff --git a/sdk/core/Azure.Core.Amqp/tests/AmqpMessageIdTests.cs b/sdk/core/Azure.Core.Amqp/tests/AmqpMessageIdTests.cs
@@ -27,5 +27,13 @@ public void CanCreateFromString()
             Assert.False(messageId.Equals(Guid.NewGuid()));
             Assert.False(messageId.Equals((object)"messageId"));
         }
+
+        [Test]
+        public void CannotCreateFromNullMessageId()
+        {
+            Assert.That(
+                () => new AmqpMessageId(null),
+                Throws.InstanceOf<ArgumentNullException>());
+        }
     }
 }
diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/api/Azure.Messaging.ServiceBus.netstandard2.0.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/api/Azure.Messaging.ServiceBus.netstandard2.0.cs
@@ -360,7 +360,7 @@ protected ServiceBusSender() { }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
         public override int GetHashCode() { throw null; }
         public virtual System.Threading.Tasks.Task<long> ScheduleMessageAsync(Azure.Messaging.ServiceBus.ServiceBusMessage message, System.DateTimeOffset scheduledEnqueueTime, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
-        public virtual System.Threading.Tasks.Task<long[]> ScheduleMessagesAsync(System.Collections.Generic.IEnumerable<Azure.Messaging.ServiceBus.ServiceBusMessage> messages, System.DateTimeOffset scheduledEnqueueTime, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
+        public virtual System.Threading.Tasks.Task<System.Collections.Generic.IReadOnlyList<long>> ScheduleMessagesAsync(System.Collections.Generic.IEnumerable<Azure.Messaging.ServiceBus.ServiceBusMessage> messages, System.DateTimeOffset scheduledEnqueueTime, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual System.Threading.Tasks.Task SendMessageAsync(Azure.Messaging.ServiceBus.ServiceBusMessage message, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual System.Threading.Tasks.Task SendMessagesAsync(Azure.Messaging.ServiceBus.ServiceBusMessageBatch messageBatch, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
         public virtual System.Threading.Tasks.Task SendMessagesAsync(System.Collections.Generic.IEnumerable<Azure.Messaging.ServiceBus.ServiceBusMessage> messages, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
@@ -404,7 +404,7 @@ public ServiceBusSessionProcessorOptions() { }
         public int MaxConcurrentSessions { get { throw null; } set { } }
         public int PrefetchCount { get { throw null; } set { } }
         public Azure.Messaging.ServiceBus.ReceiveMode ReceiveMode { get { throw null; } set { } }
-        public string[] SessionIds { get { throw null; } set { } }
+        public System.Collections.Generic.IList<string> SessionIds { get { throw null; } }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
         public override bool Equals(object obj) { throw null; }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs
@@ -1159,7 +1159,7 @@ internal async Task SetStateInternal(
         /// Throws if the messages have not been deferred.</returns>
         /// <seealso cref="DeferAsync"/>
         public override async Task<IReadOnlyList<ServiceBusReceivedMessage>> ReceiveDeferredMessagesAsync(
-            IList<long> sequenceNumbers,
+            IReadOnlyList<long> sequenceNumbers,
             CancellationToken cancellationToken = default)
         {
             IReadOnlyList<ServiceBusReceivedMessage> messages = null;
@@ -1173,7 +1173,7 @@ await _retryPolicy.RunOperation(
         }
 
         internal virtual async Task<IReadOnlyList<ServiceBusReceivedMessage>> ReceiveDeferredMessagesAsyncInternal(
-            long[] sequenceNumbers,
+            IReadOnlyList<long> sequenceNumbers,
             TimeSpan timeout)
         {
             var messages = new List<ServiceBusReceivedMessage>();

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpSender.cs
@@ -304,7 +304,7 @@ internal virtual async Task SendBatchInternalAsync(
         /// <param name="messages">The list of messages to send.</param>
         /// <param name="cancellationToken">An optional <see cref="CancellationToken"/> instance to signal the request to cancel the operation.</param>
         public override async Task SendAsync(
-            IList<ServiceBusMessage> messages,
+            IReadOnlyList<ServiceBusMessage> messages,
             CancellationToken cancellationToken)
         {
             AmqpMessage messageFactory() => AmqpMessageConverter.BatchSBMessagesAsAmqpMessage(messages);
@@ -374,8 +374,8 @@ private void OnManagementLinkClosed(object managementLink, EventArgs e) =>
         /// <param name="messages"></param>
         /// <param name="cancellationToken"></param>
         /// <returns></returns>
-        public override async Task<long[]> ScheduleMessagesAsync(
-            IList<ServiceBusMessage> messages,
+        public override async Task<IReadOnlyList<long>> ScheduleMessagesAsync(
+            IReadOnlyList<ServiceBusMessage> messages,
             CancellationToken cancellationToken = default)
         {
             long[] seqNumbers = null;
@@ -399,7 +399,7 @@ await _retryPolicy.RunOperation(async (timeout) =>
         /// <param name="cancellationToken"></param>
         /// <returns></returns>
         internal async Task<long[]> ScheduleMessageInternalAsync(
-            IList<ServiceBusMessage> messages,
+            IReadOnlyList<ServiceBusMessage> messages,
             TimeSpan timeout,
             CancellationToken cancellationToken = default)
         {
@@ -492,7 +492,7 @@ internal async Task<long[]> ScheduleMessageInternalAsync(
         /// <param name="cancellationToken"></param>
         /// <returns></returns>
         public override async Task CancelScheduledMessagesAsync(
-            long[] sequenceNumbers,
+            IReadOnlyList<long> sequenceNumbers,
             CancellationToken cancellationToken = default)
         {
             Task cancelMessageTask = _retryPolicy.RunOperation(async (timeout) =>
@@ -515,7 +515,7 @@ await CancelScheduledMessageInternalAsync(
         /// <param name="cancellationToken"></param>
         /// <returns></returns>
         internal async Task CancelScheduledMessageInternalAsync(
-            long[] sequenceNumbers,
+            IReadOnlyList<long> sequenceNumbers,
             TimeSpan timeout,
             CancellationToken cancellationToken = default)
         {

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportReceiver.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportReceiver.cs
@@ -180,7 +180,7 @@ public abstract Task DeadLetterAsync(
         /// Throws if the messages have not been deferred.</returns>
         /// <seealso cref="DeferAsync"/>
         public abstract Task<IReadOnlyList<ServiceBusReceivedMessage>> ReceiveDeferredMessagesAsync(
-            IList<long> sequenceNumbers,
+            IReadOnlyList<long> sequenceNumbers,
             CancellationToken cancellationToken = default);
 
         /// <summary>

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportSender.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Core/TransportSender.cs
@@ -57,7 +57,7 @@ public abstract ValueTask<TransportMessageBatch> CreateMessageBatchAsync(
         /// <param name="messages">The list of messages to send.</param>
         /// <param name="cancellationToken">An optional <see cref="CancellationToken"/> instance to signal the request to cancel the operation.</param>
         public abstract Task SendAsync(
-            IList<ServiceBusMessage> messages,
+            IReadOnlyList<ServiceBusMessage> messages,
             CancellationToken cancellationToken);
 
         /// <summary>
@@ -79,8 +79,8 @@ public abstract Task SendBatchAsync(
         /// <param name="messages"></param>
         /// <param name="cancellationToken"></param>
         /// <returns></returns>
-        public abstract Task<long[]> ScheduleMessagesAsync(
-            IList<ServiceBusMessage> messages,
+        public abstract Task<IReadOnlyList<long>> ScheduleMessagesAsync(
+            IReadOnlyList<ServiceBusMessage> messages,
             CancellationToken cancellationToken = default);
 
         /// <summary>
@@ -90,7 +90,7 @@ public abstract Task<long[]> ScheduleMessagesAsync(
         /// <param name="cancellationToken"></param>
         /// <returns></returns>
         public abstract Task CancelScheduledMessagesAsync(
-            long[] sequenceNumbers,
+            IReadOnlyList<long> sequenceNumbers,
             CancellationToken cancellationToken = default);
 
         /// <summary>

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Diagnostics/ServiceBusEventSource.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Diagnostics/ServiceBusEventSource.cs
@@ -285,7 +285,7 @@ public void ReceiveDeferredMessageStartCore(string identifier, int messageCount,
         }
 
         [NonEvent]
-        public virtual void ReceiveDeferredMessageStart(string identifier, IList<long> sequenceNumbers)
+        public virtual void ReceiveDeferredMessageStart(string identifier, IReadOnlyList<long> sequenceNumbers)
         {
             if (IsEnabled())
             {
@@ -371,12 +371,12 @@ public virtual void ScheduleMessagesException(string identifier, string exceptio
         }
 
         [NonEvent]
-        public virtual void CancelScheduledMessagesStart(string identifier, long[] sequenceNumbers)
+        public virtual void CancelScheduledMessagesStart(string identifier, IReadOnlyList<long> sequenceNumbers)
         {
             if (IsEnabled())
             {
                 var formattedSequenceNumbers = StringUtility.GetFormattedSequenceNumbers(sequenceNumbers);
-                CancelScheduledMessagesStartCore(identifier, sequenceNumbers.Length, formattedSequenceNumbers);
+                CancelScheduledMessagesStartCore(identifier, sequenceNumbers.Count, formattedSequenceNumbers);
             }
         }
 

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusModelFactory.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Primitives/ServiceBusModelFactory.cs
@@ -44,11 +44,21 @@ public static ServiceBusReceivedMessage ServiceBusReceivedMessage(
             DateTimeOffset enqueuedTime = default)
         {
             var amqpMessage = new AmqpAnnotatedMessage(new AmqpMessageBody(new ReadOnlyMemory<byte>[] { body }));
-            amqpMessage.Properties.CorrelationId = new AmqpMessageId(correlationId);
+
+            if (correlationId != default)
+            {
+                amqpMessage.Properties.CorrelationId = new AmqpMessageId(correlationId);
+            }
             amqpMessage.Properties.Subject = subject;
-            amqpMessage.Properties.To = new AmqpAddress(to);
+            if (to != default)
+            {
+                amqpMessage.Properties.To = new AmqpAddress(to);
+            }
             amqpMessage.Properties.ContentType = contentType;
-            amqpMessage.Properties.ReplyTo = new AmqpAddress(replyTo);
+            if (replyTo != default)
+            {
+                amqpMessage.Properties.ReplyTo = new AmqpAddress(replyTo);
+            }
             amqpMessage.MessageAnnotations[AmqpMessageConstants.ScheduledEnqueueTimeUtcName] = scheduledEnqueueTime.UtcDateTime;
 
             if (messageId != default)

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusSessionProcessor.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusSessionProcessor.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Diagnostics.CodeAnalysis;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using Azure.Core.Amqp;
@@ -94,7 +95,7 @@ internal ServiceBusSessionProcessor(
                 true,
                 plugins,
                 options.ToProcessorOptions(),
-                options.SessionIds,
+                options.SessionIds.ToArray(),
                 options.MaxConcurrentSessions,
                 options.MaxConcurrentCallsPerSession);
         }

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusSessionProcessorOptions.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Processor/ServiceBusSessionProcessorOptions.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Collections.Generic;
 using System.ComponentModel;
 using Azure.Core;
 
@@ -126,12 +127,12 @@ public int MaxConcurrentCallsPerSession
         private int _maxConcurrentCallsPerSessions = 1;
 
         /// <summary>
-        /// Gets or sets an optional list of session IDs to scope
-        /// the <see cref="ServiceBusSessionProcessor"/> to. If left
-        /// blank, the processor will not be limited to any specific
+        /// Gets an optional list of session IDs to scope
+        /// the <see cref="ServiceBusSessionProcessor"/> to. If the list is
+        /// left empty, the processor will not be limited to any specific
         /// session IDs.
         /// </summary>
-        public string[] SessionIds { get; set; }
+        public IList<string> SessionIds { get; } = new List<string>();
 
         /// <summary>
         /// Determines whether the specified <see cref="System.Object" /> is equal to this instance.

diff --git a/sdk/servicebus/Azure.Messaging.ServiceBus/src/Receiver/ServiceBusReceiver.cs b/sdk/servicebus/Azure.Messaging.ServiceBus/src/Receiver/ServiceBusReceiver.cs
@@ -423,6 +423,8 @@ private async Task<IReadOnlyList<ServiceBusReceivedMessage>> PeekMessagesInterna
             CancellationToken cancellationToken)
         {
             Argument.AssertNotDisposed(IsClosed, nameof(ServiceBusReceiver));
+            Argument.AssertAtLeast(maxMessages, 1, nameof(maxMessages));
+
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
             Logger.PeekMessageStart(Identifier, sequenceNumber, maxMessages);
             using DiagnosticScope scope = ScopeFactory.CreateScope(
@@ -898,11 +900,21 @@ public virtual async Task<IReadOnlyList<ServiceBusReceivedMessage>> ReceiveDefer
             CancellationToken cancellationToken = default)
         {
             Argument.AssertNotDisposed(IsClosed, nameof(ServiceBusReceiver));
-            Argument.AssertNotNullOrEmpty(sequenceNumbers, nameof(sequenceNumbers));
+            Argument.AssertNotNull(sequenceNumbers, nameof(sequenceNumbers));
             cancellationToken.ThrowIfCancellationRequested<TaskCanceledException>();
-            var sequenceNumbersList = sequenceNumbers.ToList();
 
-            Logger.ReceiveDeferredMessageStart(Identifier, sequenceNumbersList);
+            IReadOnlyList<long> sequenceList = sequenceNumbers switch
+            {
+                IReadOnlyList<long> alreadyList => alreadyList,
+                _ => sequenceNumbers.ToList()
+            };
+
+            if (sequenceList.Count == 0)
+            {
+                return Array.Empty<ServiceBusReceivedMessage>();
+            }
+
+            Logger.ReceiveDeferredMessageStart(Identifier, sequenceList);
             using DiagnosticScope scope = ScopeFactory.CreateScope(DiagnosticProperty.ReceiveDeferredActivityName);
             scope.AddAttribute(
                 DiagnosticProperty.SequenceNumbersAttribute,
@@ -913,7 +925,7 @@ public virtual async Task<IReadOnlyList<ServiceBusReceivedMessage>> ReceiveDefer
             try
             {
                 deferredMessages = await InnerReceiver.ReceiveDeferredMessagesAsync(
-                    sequenceNumbersList,
+                    sequenceList,
                     cancellationToken).ConfigureAwait(false);
             }
             catch (Exception exception)