Azure.Core 2.0: Address concerns about policy inheritance

[annelo-msft]

This draft PR adds tests from @AlexanderSher's Azure.Core 2.0 integration gist illustrating concerns about the inheritance of the Azure.Core HttpPipelinePolicy type from System.ClientModel PipelinePolicy.

@AlexanderSher explains the concerns in this PR comment: #42328 (comment)

[annelo-msft]

From @AlexanderSher:

TL;DR IMO, we should remove inheritance between Azure.Core types and System.ClientModel types and replace it with composition cause we can't preserve inheritance of contract and don't win much in terms of reusability, at the same time we will have all downsides of strong coupling between multiple types in two libraries.

Full version Inheritance of contract can be used in two ways: by using Azure.Core derived types inside the System.ClientModel codeflow, or to use System.ClientModel derived types inside Azure.Core codeflow. As a simple example, let's imagine custom policy that is used in the pipeline (full set of tests).

The most natural way would be to use some Azure.Core policy inside System.ClientModel pipeline. For that purpose, we will use DoNothingPolicy that simply passes execution forward:

private class DoNothingPolicy : HttpPipelinePolicy
{
    public override ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
        => pipeline.Span[0].ProcessAsync(message, pipeline.Slice(1));

    public override void Process(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
        => pipeline.Span[0].Process(message, pipeline.Slice(1));
}

Test itself would look like this:

[Test]
public async Task AzurePolicyInClientModelPipeline()
{
    ClientPipelineOptions options = new()
    {
        Transport = new MockPipelineTransport(),
    };

    ClientPipeline pipeline = ClientPipeline.Create(options,
        perCallPolicies: new[]{new DoNothingPolicy()},
        perTryPolicies: ReadOnlySpan<PipelinePolicy>.Empty,
        beforeTransportPolicies: ReadOnlySpan<PipelinePolicy>.Empty);

    using PipelineMessage message = new HttpMessage(new MemoryRequest(), ResponseClassifier.Shared);
    await pipeline.SendAsync(message);
}

This test will fail because HttpPipelinePolicy.ProcessAsync does upcasting validation and throws InvalidOperationException. I haven't found a way to workaround it from client-side code, but strictly speaking users don't have to - derived class is expected to work as good as base one.

Now, if we try to do it the other way around - pass System.ClientModel derived policy into Azure.Core pipeline - the naive attempt won't throw any exceptions:

[Test]
public async Task ClientModelPolicyInAzurePipeline()
{
    var pipeline = HttpPipelineBuilder.Build(new TestClientOptions { Transport = new MockTransport(new MockResponse(404)) });

    var context = new RequestContext();
    context.AddPolicy(new ReplaceResponseClassifierPipelinePolicy(), PipelinePosition.PerCall);

    using HttpMessage message = pipeline.CreateMessage(context);
    await pipeline.SendAsync(message, message.CancellationToken);
    Assert.IsFalse(message.Response.IsError);
}

Test assert, however, will fail, because ReplaceResponseClassifierPipelinePolicy won't be added to the pipeline - non-azure policies are simply ignored.

private class ReplaceResponseClassifierPipelinePolicy : PipelinePolicy
{
    public override void Process(PipelineMessage message, IReadOnlyList<PipelinePolicy> pipeline, int currentIndex)
    {
        message.ResponseClassifier = new CustomPipelineMessageClassifier();
        ProcessNext(message, pipeline, currentIndex);
    }

    public override async ValueTask ProcessAsync(PipelineMessage message, IReadOnlyList<PipelinePolicy> pipeline, int currentIndex)
    {
        message.ResponseClassifier = new CustomPipelineMessageClassifier();
        await ProcessNextAsync(message, pipeline, currentIndex).ConfigureAwait(false);
    }
}

private class CustomPipelineMessageClassifier : PipelineMessageClassifier
{
    public override bool TryClassify(PipelineMessage message, out bool isError)
    {
        isError = !message.Response!.Status.Equals(404);
        return !isError;
    }

    public override bool TryClassify(PipelineMessage message, Exception exception, out bool isRetriable)
    {
        isRetriable = exception == null && message.Response != null && message.Response.Status.Equals(404);
        return isRetriable;
    }
}

And if we try to wrap ReplaceResponseClassifierPipelinePolicy into Azure.Core policy to ensure it is added to the pipeline:

private class ClientPolicyWrapper : HttpPipelinePolicy
{
    private readonly PipelinePolicy _policy;

    public ClientPolicyWrapper(PipelinePolicy policy)
    {
        _policy = policy;
    }

    public override async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
    {
        await _policy.ProcessAsync(message, pipeline.Slice(1).ToArray(), 0).ConfigureAwait(false);
    }

    public override void Process(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
    {
        _policy.Process(message, pipeline.Slice(1).ToArray(), 0);
    }
}

[Test]
public async Task ClientModelPolicyWrappedForAzurePipeline()
{
    var options = new TestClientOptions { Transport = new MockTransport(new MockResponse(404)) };
    options.AddPolicy(new ClientPolicyWrapper(new ReplaceResponseClassifierPipelinePolicy()), HttpPipelinePosition.PerCall);
    var pipeline = HttpPipelineBuilder.Build(options);

    var context = new RequestContext();
    using HttpMessage message = pipeline.CreateMessage(context);
    await pipeline.SendAsync(message, message.CancellationToken);
    Assert.IsFalse(message.Response.IsError);
}

The test will fail in the same HttpPipelinePolicy.ProcessAsync upcasting validation.

Ok, lets write more sophisticated wrapper which restores back the original Azure.Core-specific pipeline:

private class AdvancedClientPolicyWrapper : HttpPipelinePolicy
{
    private readonly PipelinePolicy _policy;

    public AdvancedClientPolicyWrapper(PipelinePolicy policy)
    {
        _policy = policy;
    }

    public override async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
    {
        await _policy.ProcessAsync(message, new[]{null, new Shim(message, pipeline)}, 0).ConfigureAwait(false);
    }

    public override void Process(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
    {
        _policy.Process(message, new[]{null, new Shim(message, pipeline)}, 0);
    }

    private class Shim : PipelinePolicy
    {
        private readonly HttpMessage _message;
        private readonly ReadOnlyMemory<HttpPipelinePolicy> _pipeline;

        public Shim(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
        {
            _message = message;
            _pipeline = pipeline;
        }

        public override ValueTask ProcessAsync(PipelineMessage message, IReadOnlyList<PipelinePolicy> pipeline, int currentIndex)
        {
            return _pipeline.Span[0].ProcessAsync(_message, _pipeline.Slice(1));
        }

        public override void Process(PipelineMessage message, IReadOnlyList<PipelinePolicy> pipeline, int currentIndex)
        {
            _pipeline.Span[0].Process(_message, _pipeline.Slice(1));
        }
    }
}

With this wrapper, our test will finally pass:

[Test]
public async Task ClientModelPolicyWrappedForAzurePipelineV2()
{
    var options = new TestClientOptions { Transport = new MockTransport(new MockResponse(404)) };
    options.AddPolicy(new AdvancedClientPolicyWrapper(new ReplaceResponseClassifierPipelinePolicy()), HttpPipelinePosition.PerCall);
    var pipeline = HttpPipelineBuilder.Build(options);

    var context = new RequestContext();
    using HttpMessage message = pipeline.CreateMessage(context);
    await pipeline.SendAsync(message, message.CancellationToken);
    Assert.IsFalse(message.Response.IsError);
}

However, if we change the response code to any other error code, it will fail because Azure.Core retry policy doesn't expect PipelineMessageClassifier.TryClassify to return false:

[Test]
public async Task ClientModelPolicyWrappedForAzurePipelineV3()
{
    var options = new TestClientOptions { Transport = new MockTransport(new MockResponse(400)) };
    options.AddPolicy(new AdvancedClientPolicyWrapper(new ReplaceResponseClassifierPipelinePolicy()), HttpPipelinePosition.PerCall);
    var pipeline = HttpPipelineBuilder.Build(options);

    var context = new RequestContext();
    using HttpMessage message = pipeline.CreateMessage(context);
    await pipeline.SendAsync(message, message.CancellationToken);
    Assert.IsTrue(message.Response.IsError);
}

So inheritance of contract doesn't work in both ways. And this is just a tip of the iceberg, cause policy injection is the simplest thing that can be done here, and policies themselves are trivial.

I also want to point out that ClientModelPolicyWrappedForAzurePipelineV2 has been failing until PipelineMessageClassifierAdapter has been added, but that was not a design-level fix - it is a patch in the place that we have found, and I'm sure there will be more.

We already have a bunch of adapters to support the emulation of inheritance for the cases we know: HttpPipelineAdapter, HttpClientPipelineResponse, PipelineMessageClassifierAdapter, etc. Yes, we save some code volume on reuse of System.ClientModel types in Azure.Core, but we add good amount of new code that is very hard to read and will be quite expensive to support.

Since we already have composition (via adapters) in multiple places, I think we should fully proceed with that approach and get rid of inheritance between Azure.Core and System.ClientModel pipeline types.

Original comment: #42328 (comment)

[annelo-msft]

@AlexanderSher, in response to:

IMO, we should remove inheritance between Azure.Core types and System.ClientModel types and replace it with composition

Is your concern only with the inheritance of Azure.Core's HttpPipelinePolicy from System.ClientModel PipelinePolicy, or are there other types in the integration that you have concerns with as well?

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Core

[AlexanderSher]

@AlexanderSher, in response to:

IMO, we should remove inheritance between Azure.Core types and System.ClientModel types and replace it with composition

Is your concern only with the inheritance of Azure.Core's HttpPipelinePolicy from System.ClientModel PipelinePolicy, or are there other types in the integration that you have concerns with as well?

All the types. I can create similar tests for Request:PipelineRequest and Response:PipelineResponse pairs, but they will be more complicated.

My main premise here is that if we need to write hundreds (if not thousands) lines of code to support basic OO inheritance between types that in C# should just work, we are doing something wrong.

[annelo-msft]

All the types. I can create similar tests for Request:PipelineRequest and Response:PipelineResponse pairs, but they will be more complicated.

I agree that we have issues with the inheritance of policies, but I had thought we had resolved remaining issues with polymorphism for these types. If you can provide tests that fail for other types in the integration, that would be very helpful in assessing the feasibility of this integration approach. The list of all Azure.Core types that inherit from System.ClientModel types can be found in the APIView linked from this PR.

[github-actions]

Hi @annelo-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

[github-actions]

Hi @annelo-msft. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.