update policy copying for clone and convert (#21069)

sdk/core/Azure.Core/tests/ManagementPipelineBuilderTests.cs

@@ -0,0 +1,56 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Linq;
+using System.Reflection;
+using System.Threading;
+using Azure.Core.Pipeline;
+using Azure.Core.TestFramework;
+using Azure.ResourceManager.Core;
+using NUnit.Framework;
+
+namespace Azure.Core.Tests.Management
+{
+    public class ManagementPipelineBuilderTests
+    {
+        [TestCase]
+        public void AddPerCallPolicy()
+        {
+            var options = new ArmClientOptions();
+            var dummyPolicy = new DummyPolicy();
+            options.AddPolicy(dummyPolicy, HttpPipelinePosition.PerCall);
+            var pipeline = ManagementPipelineBuilder.Build(new MockCredential(), new Uri("http://foo.com"), options);
+
+            var perCallPolicyField = pipeline.GetType().GetField("_pipeline", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.GetField);
+            var policies = (ReadOnlyMemory<HttpPipelinePolicy>)perCallPolicyField.GetValue(pipeline);
+            Assert.IsNotNull(policies.ToArray().FirstOrDefault(p => p.GetType() == typeof(DummyPolicy)));
+        }
+
+        [TestCase]
+        public void AddPerCallPolicyViaClient()
+        {
+            var options = new ArmClientOptions();
+            var dummyPolicy = new DummyPolicy();
+            options.AddPolicy(dummyPolicy, HttpPipelinePosition.PerCall);
+            var client = new ArmClient(Guid.NewGuid().ToString(), new MockCredential(), options);
+
+            var pipelineProperty = client.GetType().GetProperty("Pipeline", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.GetProperty);
+            var pipeline = pipelineProperty.GetValue(client) as HttpPipeline;
+
+            var perCallPolicyField = pipeline.GetType().GetField("_pipeline", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.GetField);
+            var policies = (ReadOnlyMemory<HttpPipelinePolicy>)perCallPolicyField.GetValue(pipeline);
+            Assert.IsNotNull(policies.ToArray().FirstOrDefault(p => p.GetType() == typeof(DummyPolicy)));
+        }
+
+        private class DummyPolicy : HttpPipelineSynchronousPolicy
+        {
+            public int numMsgGot = 0;
+
+            public override void OnReceivedResponse(HttpMessage message)
+            {
+                Interlocked.Increment(ref numMsgGot);
+            }
+        }
+    }
+}

sdk/resourcemanager/Azure.ResourceManager.Core/src/ArmClient.cs

@@ -97,7 +97,7 @@ private ArmClient(
             DefaultSubscription = string.IsNullOrWhiteSpace(defaultSubscriptionId)
                 ? GetDefaultSubscription()
                 : GetSubscriptions().TryGet(defaultSubscriptionId);
-            ClientOptions.ApiVersions.SetProviderClient(credential, baseUri, DefaultSubscription.Id.SubscriptionId);
+            ClientOptions.ApiVersions.SetProviderClient(credential, baseUri, defaultSubscriptionId ?? DefaultSubscription.Id.SubscriptionId);
         }
 
         /// <summary>

sdk/resourcemanager/Azure.ResourceManager.Core/src/ArmClientOptions.cs

@@ -1,15 +1,13 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-using Azure.Core;
-using Azure.Core.Pipeline;
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Reflection;
-using System.Linq;
-using System.Runtime.CompilerServices;
+using Azure.Core;
+using Azure.Core.Pipeline;
 
 namespace Azure.ResourceManager.Core
 {
@@ -29,7 +27,7 @@ public sealed class ArmClientOptions : ClientOptions
         /// Initializes a new instance of the <see cref="ArmClientOptions"/> class.
         /// </summary>
         public ArmClientOptions()
-            : this(LocationData.Default, null)
+            : this(LocationData.Default)
         {
         }
 
@@ -38,64 +36,19 @@ public ArmClientOptions()
         /// </summary>
         /// <param name="defaultLocation"> The default location to use if can't be inherited from parent. </param>
         public ArmClientOptions(LocationData defaultLocation)
-            : this(defaultLocation, null)
-        {
-        }
-
-        /// <summary>
-        /// Initializes a new instance of the <see cref="ArmClientOptions"/> class.
-        /// </summary>
-        /// <param name="defaultLocation"> The default location to use if can't be inherited from parent. </param>
-        /// <param name="other"> The client parameters to use in these operations. </param>
-        /// <exception cref="ArgumentNullException"> If <see cref="LocationData"/> is null. </exception>
-        internal ArmClientOptions(LocationData defaultLocation, ArmClientOptions other = null)
         {
             if (defaultLocation is null)
                 throw new ArgumentNullException(nameof(defaultLocation));
 
-            // Will go away when moved into core since we will have directly access the policies and transport, so just need to set those
-            if (!ReferenceEquals(other, null))
-                Copy(other);
             DefaultLocation = defaultLocation;
             ApiVersions = new ApiVersions(this);
         }
 
-        private ArmClientOptions(LocationData location, IList<HttpPipelinePolicy> perCallPolicies, IList<HttpPipelinePolicy> perRetryPolicies)
-        {
-            if (location is null)
-                throw new ArgumentNullException(nameof(location));
-
-            DefaultLocation = location;
-            PerCallPolicies = new List<HttpPipelinePolicy>();
-            foreach (var call in perCallPolicies)
-            {
-                PerCallPolicies.Add(call);
-            }
-            PerRetryPolicies = new List<HttpPipelinePolicy>();
-            foreach (var retry in perRetryPolicies)
-            {
-                PerCallPolicies.Add(retry);
-            }
-            ApiVersions = new ApiVersions(this);
-        }
-
         /// <summary>
         /// Gets the default location to use if can't be inherited from parent.
         /// </summary>
         public LocationData DefaultLocation { get; }
 
-        /// <summary>
-        /// Gets each http call policies.
-        /// </summary>
-        /// <returns> A collection of http pipeline policy that may take multiple service requests to iterate over. </returns>
-        internal IList<HttpPipelinePolicy> PerCallPolicies { get; } = new List<HttpPipelinePolicy>();
-
-        /// <summary>
-        /// Gets each http retry call policies.
-        /// </summary>
-        /// <returns> A collection of http pipeline policy that may take multiple service requests to iterate over. </returns>
-        internal IList<HttpPipelinePolicy> PerRetryPolicies { get; } = new List<HttpPipelinePolicy>();
-
         /// <summary>
         /// Converts client options.
         /// </summary>
@@ -106,47 +59,12 @@ public T Convert<T>()
         {
             var newOptions = new T();
             newOptions.Transport = Transport;
-            foreach (var pol in PerCallPolicies)
-            {
-                newOptions.AddPolicy(pol, HttpPipelinePosition.PerCall);
-            }
 
-            foreach (var pol in PerRetryPolicies)
-            {
-                newOptions.AddPolicy(pol, HttpPipelinePosition.PerRetry);
-            }
+            CopyPolicies(this, newOptions);
 
             return newOptions;
         }
 
-        /// <summary>
-        /// Adds a policy for Azure resource manager client http call.
-        /// </summary>
-        /// <param name="policy"> The http call policy in the pipeline. </param>
-        /// <param name="position"> The position of the http call policy in the pipeline. </param>
-        /// <exception cref="ArgumentNullException"> If <see cref="HttpPipelinePolicy"/> is null. </exception>
-        public new void AddPolicy(HttpPipelinePolicy policy, HttpPipelinePosition position)
-        {
-            if (policy is null)
-                throw new ArgumentNullException(nameof(policy));
-
-            // TODO policy lists are internal hence we don't have access to them by inheriting ClientOptions in this Assembly, this is a wrapper for now to convert to the concrete
-            // policy options.
-            switch (position)
-            {
-                case HttpPipelinePosition.PerCall:
-                    PerCallPolicies.Add(policy);
-                    break;
-                case HttpPipelinePosition.PerRetry:
-                    PerRetryPolicies.Add(policy);
-                    break;
-                default:
-                    throw new ArgumentOutOfRangeException(nameof(position), position, null);
-            }
-
-            base.AddPolicy(policy, position);
-        }
-
         /// <summary>
         /// Gets override object.
         /// </summary>
@@ -162,24 +80,31 @@ public object GetOverrideObject<T>(Func<object> objectConstructor)
             return _overrides.GetOrAdd(typeof(T), objectConstructor());
         }
 
-        // Will be removed like AddPolicy when we move to azure core
-        private void Copy(ArmClientOptions other)
+        private static void CopyPolicies(ClientOptions source, ClientOptions dest)
         {
-            Transport = other.Transport;
-            foreach (var pol in other.PerCallPolicies)
+            var perCallPoliciesProperty = source.GetType().GetProperty("PerCallPolicies", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.GetProperty);
+            var perCallPolicies = perCallPoliciesProperty.GetValue(source) as IList<HttpPipelinePolicy>;
+
+            foreach (var policy in perCallPolicies)
             {
-                AddPolicy(pol, HttpPipelinePosition.PerCall);
+                dest.AddPolicy(policy, HttpPipelinePosition.PerCall);
             }
 
-            foreach (var pol in other.PerRetryPolicies)
+            var perRetryPoliciesProperty = source.GetType().GetProperty("PerRetryPolicies", BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.GetProperty);
+            var perRetryPolicies = perRetryPoliciesProperty.GetValue(source) as IList<HttpPipelinePolicy>;
+
+            foreach (var policy in perRetryPolicies)
             {
-                AddPolicy(pol, HttpPipelinePosition.PerRetry);
+                dest.AddPolicy(policy, HttpPipelinePosition.PerRetry);
             }
         }
 
         internal ArmClientOptions Clone()
         {
-            ArmClientOptions copy = new ArmClientOptions(DefaultLocation, PerCallPolicies, PerRetryPolicies);
+            ArmClientOptions copy = new ArmClientOptions(DefaultLocation);
+
+            CopyPolicies(this, copy);
+
             copy.ApiVersions = ApiVersions.Clone();
             copy.Transport = Transport;
             return copy;

sdk/resourcemanager/Azure.ResourceManager.Core/tests/Unit/ArmClientOptionsTests.cs

@@ -21,8 +21,6 @@ public void ValidateClone()
             Assert.IsFalse(ReferenceEquals(options1.Diagnostics, options2.Diagnostics));
             Assert.IsFalse(ReferenceEquals(options1.Retry, options2.Retry));
             Assert.IsFalse(ReferenceEquals(options1.ApiVersions, options2.ApiVersions));
-            Assert.IsFalse(ReferenceEquals(options1.PerCallPolicies, options2.PerCallPolicies));
-            Assert.IsFalse(ReferenceEquals(options1.PerRetryPolicies, options2.PerRetryPolicies));
         }
 
         [TestCase]
@@ -77,10 +75,6 @@ public void MultiClientSeparateVersions()
         public void TestClientOptionsParamCheck()
         {
             Assert.Throws<ArgumentNullException>(() => { new ArmClientOptions(null); });
-            Assert.Throws<ArgumentNullException>(() => { new ArmClientOptions(null, null); });
-
-            var options = new ArmClientOptions();
-            Assert.Throws<ArgumentNullException>(() => { options.AddPolicy(null, HttpPipelinePosition.PerCall); });
         }
 
         [TestCase]