ClientModel: Updates to ClientModel public APIs from Azure.Core 2.0 integration

[annelo-msft]

While working on the Azure.Core 2.0 integration, we found some changes we'd like to make to ClientModel APIs:

• Make RequestOptions.Apply protected based on Azure.Core 2.0: Update HttpMessage, ResponseClassifier, and RequestOptions #42262
• Remove ISerializable implementation from ClientResultException until we complete the Azure.Core 2.0 integration work
• Make value parameter nullable in PipelineMessage.SetProperty method, per Azure.Core 2.0: Update AzureKeyCredential, RetryPolicy, and StatusCodeClassifier #42329 (comment)

[azure-sdk]

API change check

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

System.ClientModel

[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.

[annelo-msft]

@AlexanderSher, it looks like your comment

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.

Is relevant to the Azure.Core 2.0 integration work and not this PR specifically?

I plan to merge this PR which addresses the ClientModel public API and is needed to unblock the library's GA release, but these are all great points you're raising about the integration work and I'll respond your comments in a separate PR thread. Thanks!

[AlexanderSher]

@AlexanderSher, it looks like your comment

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.

Is relevant to the Azure.Core 2.0 integration work and not this PR specifically?

Maybe. I'm a bit confused with the PR hierarchy in Azure.Core 2.0 workflow, so it is up to you.