Closed annelo-msft closed 9 months ago
Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.
I have no problem with the api itself but I don't see the need for it to be part of the BCL. It feels like this is a standalone package. If it's going to be maintained in this repository but shipped entirely as a separate package then there's no reason for it not to be maintained in another repository.
There are quite a lot of classes, I would probably need more explanation (preferrably some diagrams) how all these fit together, there seem to be come concept of pipelining and some transport abstractions.
It also seems to me that there there is a lot duplication of existing concepts. How are these different?
HttpPipelineRequest
and HttpRequestMessage
(and similarly for response types)MessageHeaders
and HttpHeaders
MessageBody
and HttpContent
some random thoughts:
HttpClient
and JSON serializing/deserializing?RequestOptions
, PipelineOptions
) I thought we wanted to move away from using these (remember ServicePointManager
?)Result
might be quite common and could lead to name conflicts, it feels the same as if we named System.Net.Http.HttpClient
just System.Net.Http.Client
KeyCredential
- there have been requests for better authentication support for HttpClient, such as https://github.com/dotnet/runtime/issues/91867, it is something we would like to do in 9.0, and it could be leveraged here as well to provide more auth options.I have no problem with the api itself but I don't see the need for it to be part of the BCL. It feels like this is a standalone package. If it's going to be maintained in this repository but shipped entirely as a separate package then there's no reason for it not to be maintained in another repository.
@Wraith2, to make sure I understand your comment correctly, is your suggestion that the types remain in the System.Net.ClientModel
namespace, but that the package be maintained and shipped from a repo outside dotnet/runtime?
This API feels to me like something that should be in a Microsoft.Extensions
namespace & package, FWIW.
@Wraith2, to make sure I understand your comment correctly, is your suggestion that the types remain in the
System.Net.ClientModel
namespace, but that the package be maintained and shipped from a repo outside dotnet/runtime?If that is correct, I think we are open to this outcome, especially if the community feels strongly about this.
Yes, that's right.
@Wraith2, to make sure I understand your comment correctly, is your suggestion that the types remain in the System.Net.ClientModel namespace, but that the package be maintained and shipped from a repo outside dotnet/runtime?
If that is correct, I think we are open to this outcome, especially if the community feels strongly about this.
Yes, that's right.
Thanks! I will update the issue description to make it clear that we plan to maintain and ship the package in a separate repo. We are planning to keep the System.Net.ClientModel
package source and release pipelines in the azure-sdk-for-net repo, alongside the Azure.Core
source.
There are quite a lot of classes, I would probably need more explanation (preferrably some diagrams) how all these fit together, there seem to be come concept of pipelining and some transport abstractions.
@rzikm, I agree that there's a lot here, and I am interested in understanding better how the community prefers to share information like diagrams might be best represented visually. Since I am new to the dotnet/runtime community, please don't hesitate to share examples of things that work well for this group, or to guide me in the right direction if my initial attempts aren't what the community prefers.
I will loop back with some visualizations that I think may be helpful to illustrate some of these concepts, as well as bring some more usage samples to break down the APIs into related sets of functionality.
Some initial responses to your questions:
How are these different?
HttpPipelineRequest
andHttpRequestMessage
(and similarly for response types)MessageHeaders
andHttpHeaders
MessageBody
andHttpContent
I will remove HttpPipelineRequest
and HttpPipelineResponse
from consideration for now, as we continue to evaluate the need for this as a public API.
For the latter two, we are aware of these types in the BCL, and acknowledge that the functionality is indeed similar. Our goal in introducing MessageHeaders
and MessageBody
as analogs of HttpHeaders
and HttpContent
is to abstract over different the .NET HTTP stacks, so we can support users who aren't able to upgrade easily from netstandard2.0. We do this today in Azure.Core
, and would like to maintain these abstractions in the ClientModel library to enable this for those users there as well.
- Is this for generated code only? Or should it be also friendly to users who would want to write their own clients?
Ideally, we would like to make these types friendly for developers who want to write their own clients as well as useful for generated clients, so I am interested in feedback on how we might accomplish that.
What advantages does it bring compared to writing a wrapper around HttpClient and JSON serializing/deserializing?
I will try to answer this in subsequent comments, as I show usage samples of the different related type groups.
- Is dotnet/ncl expected to maintain this?
No. Our expectation is that the Azure SDK team will maintain and release this package. I've updated the issue description to include our plans around this.
- There seem to be few properties with setters (on
RequestOptions
,PipelineOptions
) I thought we wanted to move away from using these (rememberServicePointManager
?)
I will spend some time with ServicePointManager
to try to understand it better. If you would also be willing to link some discussions related to this point, I am interested in learning more about this.
- the name
Result
might be quite common and could lead to name conflicts, it feels the same as if we namedSystem.Net.Http.HttpClient
justSystem.Net.Http.Client
I had understood that my team viewed this as a benefit, e.g. using a name that aligned with the .NET concept behind the name Result
on Task<TResult>.Result
. Are there specific APIs you're concerned that we could run into naming conflicts with?
KeyCredential
- there have been requests for better authentication support for HttpClient, such as https://github.com/dotnet/runtime/issues/91867, it is something we would like to do in 9.0, and it could be leveraged here as well to provide more auth options.
Thank you for linking this, and I would like to spend some time learning more about this as well.
To address @rzikm's request for more explanation of these types, I'm including the full text of a gist my team has put together in this comment. We don't expect we'll have time to discuss every aspect of each API in an in-person review, but we include this discussion to provide a picture of the end-to-end story of a ClientModel client, from end-user usage of the client through details of a client implementation that the types in these APIs were designed to support. It starts by introducing concepts I expect to already be familiar to the community, but are included for the purposes of grounding on terminology.
In our review, we'd like to focus on reviewing the types related to reading and writing message content as the highest priority, including ModelReaderWriter
, IModel<T>
, and IJsonModel<T>
and can discuss any other types as time allows. We are happy to answer questions posted on this issue as well.
The full text of this comment is also available in the gist: System.Net.ClientModel API
This section describes the two user groups we believe can benefit from having these APIs in the .NET ecosystem. It then describes terminology we'll use to categorize and motivate different APIs. Finally, it describes two different types of methods we expect ClientModel clients to have, in order to further explain the APIs.
The APIs in ClientModel are designed for two different groups of users: the end-users of ClientModel clients, and the authors of ClientModel clients. For this discussion, we will refer to these two user groups as "client-users" and "client-authors".
To use terminology described by Richard Lander in the The convenience of .NET series, we will use the terms convenience and control to indicate the difference between APIs designed to enable writing compact and straightforward code vs those that provide more detailed options and enable lower-level control of implemented functionality.
Finally, building on the concepts above, we introduce terminology for the two types of methods that ClientModel types are designed to enable on ClientModel clients. A service method is a method on a ClientModel client that sends a message to a cloud service, receives the service's response, and returns a value to the client-user that called the method, for use in their application. ClientModel clients can expose two different types of service methods: "convenience methods" and "protocol methods".
Convenience methods are service methods that take a strongly-typed model representing schematized data needed to communicate with the cloud service as input, and return a strongly-typed model as output. Having strongly-typed models that represent service concepts provides a layer of convenience over working with raw JSON, and unifies the experience for users of ClientModel clients when cloud services differ in wire-formats. That is, a client-user can learn the patterns for strongly-typed models that ClientModel clients provide, and use them together without having to reason about whether a cloud service represents resources using JSON or XML.
Protocol methods are service methods that provide very little convenience over the raw HTTP APIs a cloud service exposes. They have the advantage of being straightforward to generate from a service's API description, and represent request and response message bodies using types that are very thin layers over raw JSON/binary/other formats. They require users to read and understand the service's HTTP API documentation directly, rather than relying on the client to provide developer conveniences via strongly-typing service schemas. In the usage samples we'll share, we show that convience methods are implemented by calling through to the lower-level protocol methods.
This section first describes the intended pattern for using ClientModel clients in application code, and then identifies the client-user types that enable this pattern.
The following shows an example of a minimal application that could be implemented using a ClientModel client. It illustrates some usage of types intended for client-users.
using Maps;
using System;
using System.Net;
using System.Net.ClientModel;
string key = Environment.GetEnvironmentVariable("MAPS_API_KEY") ?? string.Empty;
KeyCredential credential = new KeyCredential(key);
MapsClient client = new MapsClient(new Uri("https://atlas.microsoft.com"), credential);
try
{
IPAddress ipAddress = IPAddress.Parse("2001:4898:80e8:b::189");
Result<IPAddressCountryPair> result = client.GetCountryCode(ipAddress);
IPAddressCountryPair ipCountryPair = result.Value;
Console.WriteLine($"Response status code: '{result.GetRawResponse().Status}'");
Console.WriteLine($"IPAddress: '{ipCountryPair.IpAddress}', Country code: '{ipCountryPair.CountryRegion.IsoCode}'");
}
catch (UnsuccessfulRequestException e)
{
Console.WriteLine($"Error: Response status code: '{e.Status}'");
}
We highlight the following patterns intended to be common across ClientModel clients:
GetCountryCode
)IPAddressCountryPair
)result.GetRawResponse()
)UnsuccessfulRequestException
)Types intended for use by client-users in application code are grouped in the System.Net.ClientModel
namespace. These include:
KeyCredential
- supports (1)Result<T>
- supports (2), (3), (4)Result
- supports (4)UnsuccessfulRequestException
- supports (5)This section describes ClientModel client implementation patterns that the types in the System.Net.ClientModel.Core
namespace have been designed to enable. The patterns shown here include patterns for implementing a client constructor, patterns for implementing both convienence and protocol service methods -- both illustrating usage of types intended for client-authors -- and concludes with a discussion of APIs designed to support lower-level control and flexibility for both client-users and client-authors.
The first example here continues from the application sample in the prior section. It sketches an implementation of the ClientModel client MapsClient
that was used in the prior example.
At a high level, a ClientModel client can have the following parts:
namespace Maps;
public class MapsClient
{
//
// 1. Private members intialized by the client constructor
//
//
// 2. Client constructor
//
//
// 3. Service methods
// - Convenience method
// - Protocol method
//
//
// 4. Service method helpers
//
}
The following sections will provide sample code filling in the parts of this client implementation.
In a ClientModel client constructor, client-authors create an instance of a MessagePipeline
that the client will use to send and receive messages to/from a cloud service. The constructor also stores any information needed to later create requests needed by service methods.
The following illustrates the constructor implementation for our example client:
public MapsClient(Uri endpoint, KeyCredential credential, MapsClientOptions options = default)
{
if (endpoint is null) throw new ArgumentNullException(nameof(endpoint));
if (credential is null) throw new ArgumentNullException(nameof(credential));
options ??= new MapsClientOptions();
_endpoint = endpoint;
_credential = credential;
_apiVersion = options.Version;
if (options.PerCallPolicies is null)
{
options.PerCallPolicies = new PipelinePolicy[1];
}
else
{
var perCallPolicies = new PipelinePolicy[options.PerCallPolicies.Length + 1];
options.PerCallPolicies.CopyTo(perCallPolicies.AsSpan());
}
options.PerCallPolicies[options.PerCallPolicies.Length - 1] = new KeyCredentialAuthenticationPolicy(_credential, "subscription-key");
_pipeline = MessagePipeline.Create(options);
}
Note that:
_endpoint
for use later when creating requests_credential
to use in the authentication policy and/or later request creationKeyCredentialAuthenticationPolicy
is added to pipeline options provided by the user so it can add headers for authenticating the request when it is sent in the pipelineAs discussed in prior sections, ClientModel clients provide two types of service methods: convenience methods and protocol methods. Examples for both are given in this section.
In our MapsClient
example, the following shows how the GetCountryCode
convenience method might be implemented:
public virtual Result<IPAddressCountryPair> GetCountryCode(IPAddress ipAddress, CancellationToken cancellationToken = default)
{
if (ipAddress is null) throw new ArgumentNullException(nameof(ipAddress));
RequestOptions options = cancellationToken.CanBeCanceled ?
new RequestOptions() { CancellationToken = cancellationToken } :
new RequestOptions();
Result result = GetCountryCode(ipAddress.ToString(), options);
MessageResponse response = result.GetRawResponse();
IPAddressCountryPair value = IPAddressCountryPair.FromResponse(response);
return Result.FromValue(value, response);
}
Note that this method calls through to the protocol method GetCountryCode
and passes a string instead of an IPAddress
. It receives the returned Result
, obtains the lower-level MessageResponse
, and creates a strongly-typed IPAddressCountryPair
return value from the response content before creating a Response<T>
to return to the client-user.
The common patterns highlighted here include:
MessageBody
(not shown in this sample)MessageBody
to a strongly-typed output modelThe protocol method for the GetCountryCode
operation corresponds closely to the operation as defined in the service's REST API: https://learn.microsoft.com/en-us/rest/api/maps/geolocation/get-ip-to-location?tabs=HTTP. Path and query parameters in the service API correspond to primitive parameters in the client's service method, and any content meant to go in the message body would correspond to a MessageBody
input parameter. The return type is a Result
, on which GetRawResponse()
can be called to get access to the MessageResponse.Body
property.
The following shows how the GetCountryCode
protocol method could be implemented on the example client:
public virtual Result GetCountryCode(string ipAddress, RequestOptions options = null)
{
if (ipAddress is null) throw new ArgumentNullException(nameof(ipAddress));
options ??= new RequestOptions();
options.MessageClassifier = new ResponseStatusClassifier(stackalloc ushort[] { 200 });
using ClientMessage message = CreateGetLocationRequest(ipAddress, options);
_pipeline.Send(message);
MessageResponse response = message.Response;
if (response.IsError && options.ErrorBehavior == ErrorBehavior.Default)
{
throw new UnsuccessfulRequestException(response);
}
return Result.FromResponse(response);
}
Note that this method adds a MessageClassifier
to the passed-in RequestOptions
, creates a message with a request, sends the message via the pipeline, checks whether the response is an error response and throws an exception if needed, and finally returns the Result
to the caller.
Protocol methods are public methods and can be called from convenience methods or by client-users.
The common patterns highlighted here include:
MessageClassifier
ClientMessage
and MessageRequest
specific to the service operationMessagePipeline.Send
MessageResponse.IsError
and throwing an UnsuccessfulRequestException
if neededPipelineResponse
into a Result
to return to the caller.In the example, the protocol method calls a private CreateGetLocationRequest
method to create the message. This method could be implemented as follows:
private ClientMessage CreateGetLocationRequest(string ipAddress, RequestOptions options)
{
ClientMessage message = _pipeline.CreateMessage();
options.Apply(message);
MessageRequest request = message.Request;
request.Method = "GET";
UriBuilder uriBuilder = new(_endpoint.ToString());
StringBuilder path = new();
path.Append("geolocation/ip");
path.Append("/json");
uriBuilder.Path += path.ToString();
StringBuilder query = new();
query.Append("api-version=");
query.Append(Uri.EscapeDataString(_apiVersion));
query.Append("&ip=");
query.Append(Uri.EscapeDataString(ipAddress));
uriBuilder.Query = query.ToString();
request.Uri = uriBuilder.Uri;
request.Headers.Add("Accept", "application/json");
return message;
}
Note the following common patterns:
RequestOptions
to the messageAlthough the example MapsClient
only illustrates reading response content and construct a strongly-typed output model, there is a lot of complexity in how generated model code supports writing request content as well. The next section focuses on APIs for types that handle writing input model content to requests and reading response content to create output models.
In the example MapsClient
, we've included a strongly-typed output model IPAddressCountryPair
. This represents the schema for the service's response to the Geolocate IP Operation. If you consult the REST API documentation for the response body schema, you'll see that the response schema describes a pair of values -- the input IPAddress
is paired with the CountryRegion
that the service returns to "geolocate" the IP address, i.e. to identify the country where the device using the specified IP address is located.
The following sample sketches out what an implementation of the IPAddressCountryPair
output model might look like:
public class IPAddressCountryPair : IJsonModel<IPAddressCountryPair>
{
internal IPAddressCountryPair(CountryRegion countryRegion, IPAddress ipAddress)
{
CountryRegion = countryRegion;
IpAddress = ipAddress;
}
public CountryRegion CountryRegion { get; }
public IPAddress IpAddress { get; }
internal static IPAddressCountryPair FromResponse(MessageResponse response)
{
// Read JSON from response.Body and return IPAddressCountryPair
}
public IPAddressCountryPair Read(ref Utf8JsonReader reader, ModelReaderWriterOptions options)
{
// Read JSON values and return IPAddressCountryPair
}
public IPAddressCountryPair Read(BinaryData data, ModelReaderWriterOptions options)
{
// Read JSON from BinaryData and return IPAddressCountryPair
}
public void Write(Utf8JsonWriter writer, ModelReaderWriterOptions options)
{
// Write JSON representing model to Utf8JsonWriter
}
public BinaryData Write(ModelReaderWriterOptions options)
{
// Write JSON representing model to returned BinaryData value
}
}
Common patterns highlighted here include:
IJsonModel<T>
interfaceIJsonModel<T>.Read
and IModel<T>.Read
methodsIJsonModel<T>.Write
and IModel<T>.Write
methodsIJsonModel<T>.Read
is ultimately what's called to create the strongly-typed output model from the response body in ClientModel client code, albiet typically through convenience APIs such as cast operators or by using the ModelReaderWriter
type. For input models, IJsonModel<T>.Write
is what's ultimately called to create the request body to send to the service.
Further details of the APIs used to read and write ClientModel clients' strongly-typed models can be found in ModelReaderWriter.
Much of the logic needed to handle the service response is managed in the PipelineTransport
and policies in the pipeline such as ResponseBufferingPolicy
. Today, PipelineTransport
is responsible for invoking the MessageClassifer.IsError
after the response is set on the message in order to populate the MessageResponse.IsError
property. The ResponseBufferingPolicy
does the work needed to read the content out of the response network stream into a buffered MemoryStream
, to make it easier for client-authors and client-users accessing lower-level APIs when reading the response body.
Options for configuring the message classifer and network timeouts are exposed on PipelineOptions
and RequestOptions
types. Opting-out of response buffering is uncommon, so APIs enabling this are exposed only on the ResponseBufferingPolicy
itself.
Please note that these options APIs are still in preview and subject to change.
Logic for deciding how to expose an error response to the client-user, and package successful responses into strongly typed Result<T>
return types is shown in client implementation samples in the preceeding sections.
As discussed in the introductory concepts section, some of the types in ClientModel are intended for use as conveniences, and some are designed to provide greater control in the implementation. We discuss several types that enable client extensibility and customization in this section.
Two options types are provided: a PipelineOptions
type that enables configuring the pipeline, and a RequestOptions
type that can be passed to protocol methods to change the pipeline for the duration of the service method invocation, or to change how the protocol method itself functions. Both client-users and client-authors can use these types to change client and service method behavior, and precedence rules are built into these types. Users of these types can set options values, but once they are read by the client or pipeline, the options are frozen and cannot be changed.
These APIs are still in preview, but are presented here to obtain early feedback.
Both client-authors and client-users can customize the pipeline, either for the whole client or for the duration of a service method's invocation. This is enabled by setting PipelinePolicy
instances on PipelineOptions
or RequestOptions
via the different policy members on those types. When MessagePipeline.Create
is called in a ClientModel client constructor, those policies will be built into the client's pipeline.
Implementors of custom policies inherit from PipelinePolicy
and implement its abstract Process
and ProcessAsync
methods. Both these methods take a ClientMessage
and are expected to call PipelineEnumerator.ProcessNext
or PipelineEnumerator.ProcessNextAsync
to pass control to the next policy in the pipeline. A policy implementation can modify the request (e.g. to add a header) before calling ProcessNext
, and will have access to the buffered response after control returns from the ProcessNext
call. Policies can be inserted into the pipeline before the retry policy by adding them to PipelineOptions.PerCallPolicies
or after the retry policy by adding them to PipelineOptions.PerTryPolicies
.
No. Our expectation is that the Azure SDK team will maintain and release this package. I've updated the issue description to include our plans around this.
In that case, I am not sure it should live in System.Net.* namespace. That way, people would (reasonably) expect that the code belongs to our team and that we are maintaing it (and they would file issues in this repository for it). I don't think there are any external packages providing types in that/our namespace. Tagging @dotnet/ncl to correct me if I am wrong.
I will spend some time with ServicePointManager to try to understand it better. If you would also be willing to link some discussions related to this point, I am interested in learning more about this.
There is probably no need to research ServicePointManager
deeply. Essentially any global configuration knob sooner or later hits a situation where two parties (user code, or a library that user code is using, or a library that gets used transitively) will have different opinions on what the global setting should be. And that is the reason why in .NET Core we moved away from it (it no longer affects SslStream as of .NET Core), and we wanted to finally mark it as obsolete for multiple releases.
In the .NET Framework days, static properties on ServicePointManager
were the way how to configure e.g. TLS protocols used by SslStream
or provide global certificate validation callback. I personally have investigated an issue of "Our certificate validation callback is not being called" and the reason was "because another Nuget package overwrote it".
I had understood that my team viewed this as a benefit, e.g. using a name that aligned with the .NET concept behind the name Result on Task
.Result. Are there specific APIs you're concerned that we could run into naming conflicts with?
The name Result<T>
evokes in me concepts from Functional Programming languages. In these languages, the type Result<T>
represents a core programming concept and is provided by the core libraries. See, e.g., F#, Rust, Haskell. There are 3rd party libraries that provide Result<T>
and similar types and related functionality to C#. Brief search found me e.g. https://github.com/vkhorikov/CSharpFunctionalExtensions
Even though such libraries are not mainstream in C# for us to worry about too many conflicts, I still feel that Result<T>
is still too general a name for a concept that is rather specific to System.Net.ClientModel
namespace.
This, of course, is just my personal feeling and the API Review board may have a different opinion on that.
To address @rzikm's request for more explanation of these types, I'm including the full text of a gist my team has put together in this comment.
Thanks. I will go through that later.
Our goal in introducing MessageHeaders and MessageBody as analogs of HttpHeaders and HttpContent is to abstract over different the .NET HTTP stacks, so we can support users who aren't able to upgrade easily from netstandard2.0
Can you elaborate on this? HttpHeaders and HttpContent are both in .NET Framework and netstandard2.0... how does netstandard2.0 impact an ability to abstract over these?
Result
Result
is a super general name and makes me nervous because of its generality. Can we choose a name that speaks better to what it is? My understanding is it's a union of both a result and the raw response that yielded that result. So, we can name it instead something like TaggedResult
or ClientModelResult
or ResultWithResponse
or something along those lines?
Also, why is it necessary to separate NullableResult from Result and make the latter derive from the former? Why not just have the former's members on the latter directly? If there's some kind of goal for separating nullable from non-nullable, then it doesn't make sense to me that the non-nullable one derives from the nullable one and therefore is a nullable one.
UnsuccessfulRequestException
What is the purpose of this exception type? How is this different from HttpRequestException?
public static System.Net.ClientModel.Core.MessageBody Create(System.Net.ClientModel.Core.IJsonModel<object> model, System.Net.ClientModel.ModelReaderWriterOptions? options = null) { throw null; }
Why does this need to exist? There's also a Create overload that differs from this only in that it takes an IModel instead of an IJsonModel... but an IJsonModel is an IModel. What does the extra IJsonModel overload enable? I ask because it seems unfortunate to take something like MessageBody that seems to be trying to be protocol agnostic and then tie it explicitly to JSON in API.
KeyCredential
In what situation does TryGetKey return false? The only way to create one of these is with a ctor that takes a string, and the only way to change it is an Update method that also takes a string. Is this outline missing nullable annotations, these are both actually supposed to be nullable strings, and TryGetKey returns false if the key is null?
WriteCore
What is the relationship between ModelReaderWriter's static Write methods and its static WriteCore method? And why does WriteCore take an IJsonModel rather than an IModel?
ModelReaderWriterFormat.Wire
What format is this? There are a bazillion different "wire formats".
ModelReaderWriterOptions vs ModelReaderWriterFormat
What purpose does ModelReaderWriterOptions serve? From the API outline, it looks like it's immutable and the only thing it does is wrap a ModelReaderWriterFormat, so why would something take a ModelReaderWriterOptions? Is the intent that it's a placeholder for adding more options in the future? Or is the intent that other types derive from it and add more provider-specific settings? And if the latter, how does that play well with DefaultWireOptions
, which is strongly-typed to return the base?
ModelReaderWriterOptions.DefaultWireOptions
This should be a property, not a field.
ModelReaderWriterOptions ctor vs GetOptions
These look like they do the same thing. Why have both?
System.Net.ClientModel.Core
I've always found "Core" in a namespace to be strange. How would we explain the difference to someone between what's in System.Net.ClientModel and what's in System.Net.ClientModel.Core?
I{Json}Model<out T>
The Read method returns a T, but the Write method doesn't accept one. How is this supposed to work, and related why is it ok that T is covariant?
MessageBody's implicit operators
There are three conversion operators listed here: to Stream, to BinaryData, and to ReadOnlyMemory. The former is explicit, but the latter two are implicit. Are they lossless? Are they allocation-free? Will they never throw an exception? Having these as implicit looks very fishy to me.
Also, two of them have "To" method counterparts (ToBinaryData / ToStream), but there's no ToReadOnlyMemory. Why the inconsistency?
And I thought the primary purpose of BinaryData was to unify these various forms of byte data... so why are there any overloads here related to Stream and ReadOnlyMemory rather than only having things in terms of BinaryData?
Default
In some cases there's a static default on the type itself (e.g. ModelReaderWriterOptions.DefaultWireOptions) and in some cases there's one on a different type (e.g. there's no static default on MessageClassifier but there is PipelineOptions.DefaultMessageClassifier.
PipelineOptions's static properties with setters
Do these allow for arbitrary code to set them at arbitrary times? That's a recipe for many different components in the same process causing unexpected behaviors in other unrelated components elsewhere in the process.
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("The constructors of the type being deserialized are dynamically accessed and may be trimmed.")]
What does someone do to address this in their code?
@stephentoub some answers to the questions around ModelReaderWriter
What is the relationship between ModelReaderWriter's static Write methods and its static WriteCore method? And why does WriteCore take an IJsonModel rather than an IModel?
WriteCore uses an internal implementation of an IBufferWriter which allows us to write an unknown sized object into buffers with less time and allocation since we will never have to grow the buffer.
What format is this? There are a bazillion different "wire formats".
Wire format simply means the service contract. I explain in a bit more detail here https://gist.github.com/m-nash/626adc8b6e79eaa8c835f93fa156370c#introduction
What purpose does ModelReaderWriterOptions serve? From the API outline, it looks like it's immutable and the only thing it does is wrap a ModelReaderWriterFormat, so why would something take a ModelReaderWriterOptions? Is the intent that it's a placeholder for adding more options in the future? Or is the intent that other types derive from it and add more provider-specific settings? And if the latter, how does that play well with DefaultWireOptions, which is strongly-typed to return the base?
The intent is that its a placeholder for future items. One specific item we have planned is service version.
These look like they do the same thing. Why have both?
If you want the default options this avoids unnecessary allocations.
The Read method returns a T, but the Write method doesn't accept one. How is this supposed to work, and related why is it ok that T is covariant?
Write method is on an instance which implements IModel<T>
so the T is implied by this context. The Read method returns a T because we didn't want to limit ourselves to net7.0+. If we had static interfaces Write would return void. Since we don't you need to call the read method on an instance of an object. ModelReaderWriter takes care of this for you by using Activator on the type param.
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("The constructors of the type being deserialized are dynamically accessed and may be trimmed.")] What does someone do to address this in their code?
This is a good question. For now I think our only answer is ModelJsonConverter can't be used with AOT. @eerhardt had some ideas that we could explore to improve this.
The goal of this API is to give people a bridge to be able to get the reader / writer logic of IModel while using JsonSerializer. Originally I tried to put DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
on the Type parameter for Read but since the base class doesn't have this we are unable to do that. If we had a way to do this then trimming should work.
In that case, I am not sure it should live in System.Net.* namespace. That way, people would (reasonably) expect that the code belongs to our team and that we are maintaing it (and they would file issues in this repository for it). I don't think there are any external packages providing types in that/our namespace.
I have a personal preference for System.ClientModel since it has nice symmetry with System.ComponentModel and System.ServiceModel, but I understand the concern that @stephentoub and @KrzysztofCwalina share that users would think System.ClientModel holds building blocks for UI clients. I hear your concern though, @rzikm, that .NET users might be confused and file issues in this repo for dotnet/ncl to address. I'm not very familiar with how the process around this works and what the negative impact of it would be - can you help me build that context to better understand the tradeoff there?
Essentially any global configuration knob sooner or later hits a situation where two parties (user code, or a library that user code is using, or a library that gets used transitively) will have different opinions on what the global setting should be.
PipelineOptions
's static properties with settersDo these allow for arbitrary code to set them at arbitrary times? That's a recipe for many different components in the same process causing unexpected behaviors in other unrelated components elsewhere in the process.
Got it, and thank you for sharing this extra context for my understanding. I agree it seems dangerous if different libraries start setting static properties on PipelineOptions
differently. I believe we can remove the static DefaultMessageClassifier
, and will update the API in the description with this if I am correct. My understanding is that for others, like logging and retries, these are relatively benign in the sense that they don't fundamentally change how the client works, and since we've had them in Azure.Core for years without negative feedback, we may end up wanting to keep those. Other static settings I'll look at closely -- for me, there are remaining questions I want to answer in the e2e options story, so I view these as more in flux than other APIs in the ClientModel group. Thanks for this feedback that I'll add to the list of items to resolve as we work to complete that story!
Can you elaborate on this? HttpHeaders and HttpContent are both in .NET Framework and netstandard2.0... how does netstandard2.0 impact an ability to abstract over these?
For both of these, I think it's largely about the tradeoff between having a coherent collection of APIs in one package/area vs. what might appear to users as a mish-mash of types from different parts of the ecosystem. Sometimes, a mash-up is worth the cost because of benefits like code reuse and the ability to exchange data, but in the case of headers and content abstractions, I'm not sure that tradeoff is worth it.
What is the purpose of this exception type? How is this different from HttpRequestException?
I agree UnsuccessfulRequestException
looks similar to HttpRequestException
, especially with Status
being the one property added over the base Exception
. My concern about using HttpRequestException
instead of a type in ClientModel would include the following: it essentially locks us in to HTTP where I think we'd like to remain flexible to pivot into other protocols later if needed; we would prefer to have a constructor that takes MessageResponse
for convenient initialization of the exception from the response in client code; and that in Azure.Core we expose the full response as a member on the exception, for the case where callers need access to full protocol details, and we might want to add that to the ClientModel exception as well in the future. I'm open to naming suggestions on this one - I'm still not 100% happy with this name.
Result
is a super general name and makes me nervous because of its generality. Can we choose a name that speaks better to what it is? My understanding is it's a union of both a result and the raw response that yielded that result. So, we can name it instead something likeTaggedResult
orClientModelResult
orResultWithResponse
or something along those lines?
We are open to naming suggestions here if you have them! One constraint I'd like to put on suggestions is that we would like a single-word name for these types since they show up in all client method signatures, which are already getting fairly busy and complex.
Also, why is it necessary to separate
NullableResult
fromResult
and make the latter derive from the former? Why not just have the former's members on the latter directly? If there's some kind of goal for separating nullable from non-nullable, then it doesn't make sense to me that the non-nullable one derives from the nullable one and therefore is a nullable one.
Let me spend some more time looking at this one and bring you either a rationale or an alternate design. This is on our list of areas we'd like to iron out in the e2e story and still in flux, so again, thank you for this feedback as inputs to our work in this area!
Why does this need to exist? There's also a Create overload that differs from this only in that it takes an
IModel
instead of anIJsonModel
... but anIJsonModel
is anIModel
. What does the extraIJsonModel
overload enable? I ask because it seems unfortunate to take something likeMessageBody
that seems to be trying to be protocol agnostic and then tie it explicitly to JSON in API.
Yes, I like the observation that it ties it to a protocol. Let me spend some time with this and see if I'm able to remove that overload, and if not, why it is needed.
In what situation does
TryGetKey
return false? The only way to create one of these is with a ctor that takes a string, and the only way to change it is an Update method that also takes a string. Is this outline missing nullable annotations, these are both actually supposed to be nullable strings, and TryGetKey returns false if the key is null?
IIRC, this was part of a design iteration where we were trying to avoid string-based properties and using TryGet methods as a replacement. I want to confirm with our Identity team that there isn't a case where TryGet is needed, but if that is correct, I will change this back to a property.
I've always found "Core" in a namespace to be strange. How would we explain the difference to someone between what's in
System.Net.ClientModel
and what's inSystem.Net.ClientModel.Core
?
The difference as I understand it is that the types in System.Net.ClientModel
are intended for users of ClientModel clients and that types in System.Net.ClientModel.Core
are intended for client-author users and client-users who want lower-level control of client functionality. I don't know that we're strongly tied to the name "Core", though, so if you have suggestions to better align with that description, we may be open to them.
There are three conversion operators listed here: to
Stream
, toBinaryData
, and toReadOnlyMemory
. The former is explicit, but the latter two are implicit. Are they lossless? Are they allocation-free? Will they never throw an exception? Having these as implicit looks very fishy to me.Also, two of them have "To" method counterparts (ToBinaryData / ToStream), but there's no ToReadOnlyMemory. Why the inconsistency?
And I thought the primary purpose of
BinaryData
was to unify these various forms of byte data... so why are there any overloads here related toStream
andReadOnlyMemory
rather than only having things in terms ofBinaryData
?
I will reevaluate these APIs in light of your comment and get back to you -- I think this is good feedback. I have an answer now for the last question, though, because I initially tried to write this entirely in terms of BinaryData
and failed. The reason we have Stream
overloads and cannot use BinaryData
alone is that BinaryData
necessarily buffers data in memory, and we have scenarios where we need to pass a network stream directly to an end-user in a client API.
These names are very general. In general, single word names (Result
, Result<T>
, IModel<T>
) are reserved for System itself, since single word names are very likely to conflict with some other project/package's local types and if there's going to be a conflict it may as well be a big'un. So for the single-word ones especially it would be good to come up with some sort of qualifier.
ModelReaderWriter.WriteCore
The "Core" suffix is usually only found on protected members, being the virtual member for a Template Method Pattern implementation. It could use a better name, like WriteJson
to avoid confusion with this pattern.
ModelReaderWriterFormat
I was going to ask why it wasn't an enum, but it looks like it's what we call a strongly typed string in FDGv3. In that case...
ModelReaderWriterFormat.Json / .Wire / ...
These should be properties, not fields. Basically, fields should never appear in public API (unless there's a complicated, compelling reason). Properties that have no body other than return (s)_backingField
will get inlined by the JIT, so they're field-fast at runtime. But if they're declared as a field they can never be converted to lazy initialization or any other thing that may be desired in the future.
- public static readonly System.Net.ClientModel.ModelReaderWriterFormat Json = ...;
- public static readonly System.Net.ClientModel.ModelReaderWriterFormat Wire = ...;
+ public static System.Net.ClientModel.ModelReaderWriterFormat Json { get; } = ...;
+ public static System.Net.ClientModel.ModelReaderWriterFormat Wire { get; } = ...;
ModelReaderWriterFormat.Wire
Since JSON is a wire format, I don't know what the difference between Json and Wire is. Perhaps this should be "Binary" or "Intrinsic" or "Internal" or something?
ModelReaderWriterOptions.DefaultWireOptions
Should be a property.
ModelReaderWriterOptions(ModelReaderWriterFormat format) and static ModelReaderWriterOptions GetOptions(ModelReaderWriterFormat format)
Only one of these two patterns should be used at a time, especially with the same signature. Since ModelReaderWriterOptions isn't sealed
, it seems like the constructor is the right pattern to use.
NullableResult
This is a very general name and feels likely to conflict with other namespaces used by developers trying to use this one.
RequestOptions
This is an extremely general name, and I strongly advise renaming it.
Result
This is an absurdly (needed a word more extreme than "extreme") general name, and essentially must be changed.
UnsuccessfulRequestException
Failed feels more in line with our normal exception names, though I don't have a guideline handy for that, just a gut feeling. Unless there are things that are not successful that you believe aren't the same as failed.
The name also feels a bit over-general, but at least the constructors tie it to this namespace and it won't be "abused" by random projects that simply find the name convenient.
namespace System.Net.ClientModel.Core
"Core" isn't a normal namespace name under System; again, it's mostly just used as the suffix for Template Method Pattern protected virtual
s. It'd be good to come up with a better name.
ClientMessage.CancellationToken
I'm not sure if there's any precedent for a settable CancellationToken property... CancellationTokens are usually method/ctor parameters.
IJsonModel
For every interface there's a question "should this be a base class instead?" A good interface describes capability. This seems to be "I can read and write myself as JSON", which meets that definition, but it's named like a type instead of a capability. Granted, "IJsonWritable" is less... concise-sounding, but the "-able" suffix is a good sniff test for interfaces.
IModel
Both the previously said Prefer a name ending in "-able"
and Avoid one-word type names
apply here, and there's also room for confusion with this because Entity Framework appears to have an IModel (though non-generic). Since EF and ClientModel being in the same project seems somewhat reasonable, this name should be avoided to avoid user confusion.
KeyCredentialAuthenticationPolicy.ctor
Assuming this is about HTTP headers, it's unclear to me if string header
is the name of the header, or the value of the header. headerName
or headerValue
as the name here would provide clarity.
MessageBody
This is a pretty general name, and feels like it could reasonably conflict with other types in namespaces utilized by developers using this namespace.
MessageBody
This type is IDisposable, and has an implicit conversion to ReadOnlyMemory. Is the answer returned from the conversion invalidated by being disposed? If so, that's a very dangerous design and should be rethought.
MessageBody's
public static implicit operator System.ReadOnlyMemory<byte>
Framework Design Guidelines 3rd edition, 2nd edition, and probably even 1st edition say DO provide methods with friendly names that correspond to each overloaded operator.
. For this one it would be ToReadOnlyMemory
.
FDG also says AVOID defining operator overloads, except in types that should feel like primitive (built-in) types.
I, personally, don't think that the conversion operators here are justified.
public abstract System.Threading.Tasks.Task WriteToAsync(System.IO.Stream stream, System.Threading.CancellationToken cancellationToken);
This is the first abstract method that I've noticed that had input parameters, so I'll mention it here and be done with it: We've generally found that abstract
members should not be public
, unless they take no parameters, and recommend the Template Method Pattern instead. For this method, it would look like
public Task WriteToAsync(Stream stream, CancellationToken cancellationToken)
{
if (stream is null)
throw new ArgumentNullException(nameof(stream));
if (!stream.CanWrite)
throw new ArgumentException("Stream must be writable", nameof(stream));
return WriteToAsyncCore(stream, cancellationToken);
}
protected abstract Task WriteToAsyncCore(Stream stream, CancellationToken cancellationToken);
This means that a) all derived types handle these failure cases consistently, and b) they don't have to do so with copy/paste. (and in this case there's also c) that argument validation for an async method was correctly done "outside" the task, instead of "inside" it)
The more complicated the argument validation is the more beneficial the pattern.
It also applies to public virtual
, like MessageClassifier.IsError, unless it's intentional that some derived types would accept null and others would reject null.
MessageRequest
I'm not sure I understand why anything on this type is virtual (abstract), and everything is. Since there's no example where someone derives from it I'm having trouble understanding this type.
It's also a very general name.
MessageResponse
I'm similarly not understanding why this type has so much virtualization going on. And it has a very general name.
ModelJsonConverter
This isn't a JsonConverter for (I)Models, but is a converter for (I)JsonModels, so JsonModelConverter seems the more appropriate name. Or JsonModelJsonConverter.
PipelineEnumerator, and everything else with Pipeline in the name
There's an existing namespace, System.IO.Pipelines. These types don't have anything to do with an existing notion of pipelines, and so I wonder if a better (or disambiguated) word can be found.
PipelineOptions
This has a lot of static properties that have setters. That's generally an anti-pattern to be avoided at nearly all cost, as the universe gets wonky when different components in an application have opinions. If they have to exist, they should probably only allow being set once.
it essentially locks us in to HTTP where I think we'd like to remain flexible to pivot into other protocols later if needed
How much of a design goal is this / which other protocols did you have in mind? From the API shape, this looks quite biased towards HTTP (I don't think they're all transferrable concepts):
In response to the comments and suggestions shared here, we've made some updates to these APIs, including:
Many thanks for all the helpful feedback posted in this issue! We look forward to further discussion of these APIs.
Thanks, @annelo-msft.
I'll go through the feedback / resulting changes in more detail, but immediate reaction is we're still going to need to do something about Result. We can't ship that in System.* with its current name (such a common word) / shape (deriving from NullableResult, which I do not understand).
Notes from our review:
.Core
is a bit unfortunate because it implies that its critical, crucial, important etcActivator.CreateInstance
and GetCustomAttributes()
@stephentoub, in response to your comment:
immediate reaction is we're still going to need to do something about Result. We can't ship that in System.* with its current name (such a common word) / shape (deriving from NullableResult, which I do not understand).
I understand the concern about this. We will follow up about the shape, but we have put together some possible names that we can discuss further to get your feedback on.
Changing the name of Result<T>
and related "result" types will likely have implications for the names of other types in the ClientModel namespaces. Because of this, I've put together a set of options we can compare on our way to finding a good solution.
The lines in the table below signify:
The column headers show the corresponding Azure.Core types we're looking to create analogs of. Empty squares indicate no change from above.
Response<T> |
NullableResponse<T> |
n/a | Response |
HttpMessage |
HttpPipeline |
Notes | |
---|---|---|---|---|---|---|---|
One word candidates | Model<T> Output<T> |
Nullable... |
ResponseMessage OutputMessage |
PipelineResponse MessageResponse |
PipelineMessage ClientMessage |
ClientPipeline MessagePipeline |
Using "Output" name could let us rename ResponseBodyContent to InputContent |
Two word candidates | ClientResponse<T> ModelResponse<T> OutputModel<T> |
Nullable... |
ResponseMessage OutputMessage |
||||
Current candidate | Result<T> |
NullableResult<T> |
Result ResultMessage |
@terrajobst, in response to the comments you captured here, we've made a number of updates including type renames and removing the ModelReaderWriterFormat
type and overloads that took it as a parameter. The API proposal in this issue's description has been updated with these changes.
Notes from today:
IModel<T>
a more specialized name?
IPersistableModel<T>
, ISerializableModel<T>
, IModelReaderWriter<T>
?Wire
feels odd as it's presented as an alternative to JSON
and XML
, which are both considered wire formats too
Wire
property?ModelReaderProxyAttribute
ModelReaderWriterAttribute
or ModelProxyAttribute
System.Net.ClientModel
System
is the best home for these APIsSystem.ClientModel
is a better name as it's unrelated to NCLSystem.Net.ClientModel.Core
.Core
but we think .Primitives
would be a better fit as it as generally non-customer facing building blocks@terrajobst, in response to comments captured here, we've made a number of updates, including:
System.Net.ClientModel
-> System.ClientModel
System.Net.ClientModel.Core
-> System.ClientModel.Primitives
IModel<T>
-> IPersistableModel<T>
ModelReaderProxyAttribute
-> PersistableModelProxyAttribute
ModelReaderWriterOptions.Wire
propertyIModel<T>.GetWireFormat
-> IPersistableModel<T>.GetFormatFromOptions
In response to @stephentoub's comment, we've tentatively renamed the Result<T>
types to OutputMessage<T>
and similar, although as mentioned in this comment we would very much prefer if we could find an acceptable one-word name for these types, given their prevalence in ClientModel client APIs, as well as their tendency to hold generic types for T
that are three+ words long. We risk the return type wrapping in some editors.
In response to comments from @GrabYourPitchforks, we've made some updates to ClientRequestException
APIs, but expect to iterate on these further as we continue our investigations on this one.
We have also made other updates based on feedback from @KrzysztofCwalina.
The API proposal in this issue's description has been updated with these changes.
Notes from last review:
KeyCredential
Update
method rather than a setter is to match the pattern of other credential types who have Update
methods taking multiple parameters in order to allow atomic updates.ToString()
doesn't expose the key (already the case)out
parameter, the inverse of Update
.OutputMessage
/NullableOutputMessage
Result<T>
, which these are. ClientRequestException
GetRawResponse()
virtual? It's being passed in.ISerializable
related stuff is fine, because it's an existing typeLet me know if I missed antyhing.
I'm under the impression that this is largely done.
@KrzysztofCwalina @annelo-msft please let me know if you believe otherwise.
Background and motivation
The Azure SDK provides .NET clients that enable idiomatic development of .NET applications that communicate with Azure services. Many of our clients are created with code generation tools, and use concepts that the .NET platform doesn't necessarily provide features for, such as retry logic and credentials for authenticating with cloud services. To avoid generating the code for these concepts into every client library, we have created a shared library called Azure.Core that our generated clients depend on. We would now like to be able to create generated clients that can communicate with any cloud service, not only those in Azure. We believe putting these client building block types in a
System.
namespace is a good way to put them in a neutral bucket.We worked with @terrajobst and @stephentoub to choose the base namespace
System.Net.ClientModel
for these types. We choseClientModel
to indicate that the types are building blocks for clients that call cloud services and included theNet
prefix to indicate that these aren't UI-client types.We plan to maintain the source for the proposed
System.Net.ClientModel
package, as well as the release pipelines needed to publish the package to NuGet, in the Azure/azure-sdk-for-net repo.API Proposal
Below is the full API listing for the types we would like to ship to enable generation of an MVP client.
Depending on the needs of our users we may chose to sequence release of these APIs over multiple package releases, where subsets of types can be released independently. We will highlight which APIs are closer to release during the in-person review.
API Usage
This sample illustrates a brief client implementation, and is followed by a sample showing application code that could be written using this client. This client can send an authenticated request to the Azure Maps service geolocate endpoint.
The following sample illustrates a minimal application built using the client defined above.
Alternative Designs
No response
Risks
No response