Add Convenience Types to System.ClientModel library #41009
Conversation
…feature/core-experiment
annelo-msft
requested review from
JoshLove-msft and
christothes
as code owners
| // Note: this is GetValue instead of GetKey to allow consistent naming | ||
| // across credential types. For example, for NamedKeyCredential, we would have | ||
| // GetValues with two out params to enable atomicity in reading credential values. | ||
| public string GetValue() => Volatile.Read(ref _key); |
There was a problem hiding this comment.
In the Azure.Core types I think we purposely didn't expose methods/properties for the key values due to security concerns. Is this not a concern?
There was a problem hiding this comment.
Discussed offline - we will move to Deconstruct APIs here, as it will give us consistency across credential types as new types like NamedKeyCredential come online, pending a strong concern from the BCL team (Anne to dig up context on the prior concern).
…to feature/core-experiment
annelo-msft
requested review from
a team,
Aashish93-stack,
abhinav-ghai,
johngallardo,
schaabs and
sjiherzig
as code owners
annelo-msft
requested review from
KarishmaGhiya,
ahsonkhan,
billwert,
mpodwysocki and
pvaneck
|
Bad merge - will open new PR |
|
Replaced by #41016 |
| private const string DefaultMessage = "Service request failed."; | ||
|
|
||
| private readonly PipelineResponse? _response; | ||
| private int _status; |
There was a problem hiding this comment.
why do we need both the response and status? PipelineResponse already has status.
| return messageBuilder.ToString(); | ||
| } | ||
|
|
||
| private static void BufferResponse(PipelineResponse response) |
There was a problem hiding this comment.
This method should not be here. I think many callers will want to buffer a response, and so maybe we just add this helper method to PipelineResponse?
|
|
||
| private static void BufferResponse(PipelineResponse response) | ||
| { | ||
| if (response.ContentStream is null) |
There was a problem hiding this comment.
should this also check if the response is not already buffered?
|
|
||
| namespace System.ClientModel; | ||
|
|
||
| public abstract class ClientResult |
There was a problem hiding this comment.
why is this abstract given no members are abstract?
| public abstract class PipelineResponse : IDisposable | ||
| { | ||
| // TODO(matell): The .NET Framework team plans to add BinaryData.Empty in dotnet/runtime#49670, and we can use it then. | ||
| private static readonly BinaryData s_emptyBinaryData = new(Array.Empty<byte>()); |
There was a problem hiding this comment.
BinaryData.Empty is already there.
5 participants
Add types in the System.ClientModel namespace -- i.e. those for end-users of ClientModel clients -- to System.ClientModel library. This includes:
ClientResult,ClientResult<T>andOptionalClientResult<T>KeyCredentialClientRequestExceptionIt also includes
PipelineResponse, which is needed to support most of these types.A discussion of the types contained in this PR can be found in the System.ClientModel README.