-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
update policy copying for clone and convert #21069
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. separate question, I think we should make Location.Default settable. Currently is fixed to WestUs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
} | ||
|
||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should remove this before GA. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make these into helper methods GetPerCallPolicy() and GetPerRetryPolicy() and have more flexible test combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://dev.azure.com/azure-mgmt-ex/DotNET%20Management%20SDK/_workitems/edit/5846