ClientModel: Convince ourselves that the virtual problem isn't a concern

[annelo-msft]

At a high level

• If we newslot a property that is virtual in Azure.Core, if a type derives from the Azure.Core type and doesn't call the base type in System.ClientModel, values on the System.ClientModel type may not get set correctly.
• When properties on PipelineResponse don't get set, policies can work incorrectly and this can cause breaking changes to existing Azure.Core-based clients.

A simple description of how this problem works is described here: https://gist.github.com/annelo-msft/73d14d238ecc61ddb287894067469598

In our code base, I discovered this when:

1. Storage test sets MockResponse on message in its MockTransport
2. Azure.Core's TransportPolicy sets IsError on Response - which sets it on MockResponse only and not in base PipelineResponse type
3. ClientModel RetryPolicy checks IsError to determine whether or not to retry
   a. IsError is set to true on MockResponse, but false on base PipelineResponse
4. Incorrect behavior happens as a result of incorrect logic in ClientModel RetryPolicy, which only uses values on PipelineResponse

Generalizing this:

• Any virtual member on an Azure.Core type can be overridden by a subtype
• If it is not also virtual on the base type, the override doesn't propagate up to the base type member, so the ClientModel values may not get set
• If ClientModel-only logic depends on the value of the base type member, it can be incorrect

Concerns I have as a result of this

• I think we have to make ClientModel members virtual if Azure.Core members are virtual, which is unfortunate for the ClientModel UX
  • i.e. we cannot newslot to add virtual in Azure.Core and make the ClientModel member non-virtual
• Do we have an unsolvable problem in the case where we have to newslot the member in order to change the type?
  • e.g. Response.Headers/PipelineResponse.Headers
  • e.g. Request.Uri, Request.Method, Request.Content, and Request.Headers

Full API description of Request and Response types

[KrzysztofCwalina]

I spent some time thinking about this. It seems to me like the most tricky case is a virtual get-set property on an existing subtype. For example, the Request.Uri property. It seems to me that we can make everything work correctly, if we: a) have just ome source of truth (a filed in the subclass), b) seal Core property/methods in the subclass. For example:

public abstract class PipelineRequest
{
    public Uri Uri {
        get => UriCore;
        set => UriCore = value;
    }

    protected abstract Uri UriCore { get; set; }
}

public abstract class Request : PipelineRequest
{
    RequestUriBuilder? _uri;
    public new virtual RequestUriBuilder Uri {
        get => _uri ??= new RequestUriBuilder();
        set => _uri = value;
    }

    protected sealed override Uri UriCore {
        get => Uri.ToUri();
        set => Uri.Reset(value);
    }
}

public class RequestUriBuilder
{
    public Uri ToUri() => throw new NotImplementedException();
    public void Reset(Uri value) => throw new NotImplementedException();
}

[KrzysztofCwalina]

In the case of get-only properties (e.g. Response.Content) where the type of the property does not change in the subclass, we should do what the notes say, i.e. make the property virtual.

In the case of get-only properties (e.g. Response.Content) where the type of the property does change in the subclass, we should do what the notes say, i.e. use protected Core method.

[KrzysztofCwalina]

Let's implement the combinations of the recommendations (from the table) and the one I added for get-set properties (Request.Uri). Hopefully these will cover all the cases. if you run into another case that is not discussed in the issue, let me know

[annelo-msft]

• We will change the GetXxCore and SetXxCore methods on PipelineRequest and PipelineResponse to XxCore properties.
• Unfortunately, I don't think we are able to seal the XxCore properties as described above, because of the way we use adapters for ClientModel-internal types in the Azure.Core integration. However, since the XxCore methods are new, no one is currently overriding them in a subtype. Since it would be quite advanced to provide derived types of Azure.Core Request and Response, I think we are safe to say that if you override one of the XxCore methods and make it not call through to the template property (the Xx property itself), this is a bug due to not following intended usage patterns. I would prefer if we could have the compiler enforce this for us, but I think this would require making request and response transport implementations in ClientModel public, and I don't think that would be a good trade-off.

[KrzysztofCwalina]

Could you link to adapter code that illustrates how the adapter overrides (or has to override) the UriCore property?

[annelo-msft]

Could you link to adapter code that illustrates how the adapter overrides (or has to override) the UriCore property?

I put a simplification of the problem as I understand it in this gist:

virtual_problem_uri.cs

The adapter implementation in the Azure.Core 2.0 integration PR branch is here:

1. Azure.Core Request newslots Uri
2. Azure.Core Request overrides UriCore -- calling through to the virtual public property on Request
3. Azure.Core internal Request sutype HttpClientTransportRequest UriCore implementation -- the internal implementation must keep the RequestUriBuilder property on the Azure.Core Request and the Uri property on the ClientModel pipeline request in sync with each other so the same value for Uri is available in both the Azure.Core type and the ClientModel type.

[annelo-msft]

Completed with #42010