From 42aceaf14a8797d34987f19d489547c360134659 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 08:53:31 -0800 Subject: [PATCH 01/17] WIP --- sdk/core/System.ClientModel/CHANGELOG.md | 2 + sdk/core/System.ClientModel/README.md | 38 +++++- .../src/Convenience/ClientRequestException.cs | 106 ++++++++++++++++ .../src/Convenience/ClientResult.cs | 72 +++++++++++ .../src/Convenience/ClientResultOfT.cs | 18 +++ .../src/Convenience/KeyCredential.cs | 38 ++++++ .../Convenience/OptionalClientResultOfT.cs | 24 ++++ .../src/Internal/ClientUtilities.cs | 91 ++++++++++++++ .../src/Message/MessageHeaders.cs | 24 ++++ .../src/Message/PipelineResponse.cs | 89 ++++++++++++++ .../src/System.ClientModel.csproj | 3 +- .../tests/Convenience/ClientResultTests.cs | 113 ++++++++++++++++++ .../TestFramework/Mocks/MockClientResult.cs | 14 +++ .../TestFramework/Mocks/MockErrorResult.cs | 22 ++++ .../TestFramework/Mocks/MockMessageHeaders.cs | 41 +++++++ .../Mocks/MockOptionalClientResult.cs | 27 +++++ .../Mocks/MockPersistableModel.cs | 39 ++++++ .../Mocks/MockPipelineResponse.cs | 64 ++++++++++ 18 files changed, 821 insertions(+), 4 deletions(-) create mode 100644 sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs create mode 100644 sdk/core/System.ClientModel/src/Convenience/ClientResult.cs create mode 100644 sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs create mode 100644 sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs create mode 100644 sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs create mode 100644 sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs create mode 100644 sdk/core/System.ClientModel/src/Message/MessageHeaders.cs create mode 100644 sdk/core/System.ClientModel/src/Message/PipelineResponse.cs create mode 100644 sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResult.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockMessageHeaders.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResult.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPersistableModel.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs diff --git a/sdk/core/System.ClientModel/CHANGELOG.md b/sdk/core/System.ClientModel/CHANGELOG.md index 01512a508935..8458be1cbe50 100644 --- a/sdk/core/System.ClientModel/CHANGELOG.md +++ b/sdk/core/System.ClientModel/CHANGELOG.md @@ -4,6 +4,8 @@ ### Features Added +- Initial release of client building block types, including Pipeline, Message, and Options types. + ### Breaking Changes ### Bugs Fixed diff --git a/sdk/core/System.ClientModel/README.md b/sdk/core/System.ClientModel/README.md index 1382a4d3b345..af5938497f7e 100644 --- a/sdk/core/System.ClientModel/README.md +++ b/sdk/core/System.ClientModel/README.md @@ -1,6 +1,8 @@ # System.ClientModel library for .NET -`System.ClientModel` provides shared primitives, abstractions, and helpers for .NET service client libraries. +`System.ClientModel` contains building blocks for communicating with cloud services. It provides shared primitives, abstractions, and helpers for .NET service client libraries. + +`System.ClientModel` allows client libraries built from its components to expose common functionality in a consistent fashion, so that once you learn how to use these APIs in one client library, you'll know how to use them in other client libraries as well. [Source code][source] | [Package (NuGet)][package] @@ -14,22 +16,46 @@ it will be installed for you when you install one of the client libraries using Install the client library for .NET with [NuGet](https://www.nuget.org/packages/System.ClientModel). ```dotnetcli -dotnet add package System.ClientModel +dotnet add package System.ClientModel --prerelease ``` ### Prerequisites None needed for `System.ClientModel`. +### Authenticate the client + +The `System.ClientModel` preview package provides a `KeyCredential` type for authentication. + ## Key concepts The main shared concepts of `System.ClientModel` include: +- Configuring service clients (`RequestOptions`). +- Accessing HTTP response details (`OutputMessage`, `OutputMessage`). +- Exceptions for reporting errors from service requests in a consistent fashion (`ClientRequestException`). - Providing APIs to read and write models in different formats. ## Examples -### Simple ModelReaderWriter usage +### Send a message using the MessagePipeline + +A rudimentary client implementation is as follows: + +```csharp +KeyCredential credential = new KeyCredential(key); +MessagePipeline pipeline = MessagePipeline.Create(options, new KeyCredentialAuthenticationPolicy(credential, "Authorization", "Bearer")); +ClientMessage message = pipeline.CreateMessage(options, new ResponseStatusClassifier(stackalloc ushort[] { 200 })); +MessageRequest request = message.Request; +request.SetMethod("POST"); +var uri = new RequestUri(); +uri.Reset(new Uri("https://www.example.com/")); +request.Uri = uri.ToUri(); +pipeline.Send(message); +Console.WriteLine(message.Response.Status); +``` + +### Read and write persistable models As a library author you can implement `IPersistableModel` or `IJsonModel` which will give library users the ability to read and write your models. @@ -51,6 +77,12 @@ string json = @"{ OutputModel? model = ModelReaderWriter.Read(BinaryData.FromString(json)); ``` +## Troubleshooting + +You can troubleshoot `System.ClientModel`-based clients by inspecting the result of any `ClientRequestException` thrown from a pipeline's `Send` method. + +## Next steps + ## Contributing This project welcomes contributions and suggestions. Most contributions require you to agree to a Contributor License Agreement (CLA) declaring that you have the right to, and actually do, grant us the rights to use your contribution. For details, visit https://cla.microsoft.com. diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs new file mode 100644 index 000000000000..da92deb1a819 --- /dev/null +++ b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs @@ -0,0 +1,106 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel.Internal; +using System.ClientModel.Primitives; +using System.Globalization; +using System.Runtime.Serialization; +using System.Text; +using System.Threading; + +namespace System.ClientModel; + +[Serializable] +public class ClientRequestException : Exception, ISerializable +{ + private const string DefaultMessage = "Service request failed."; + + private readonly PipelineResponse? _response; + private int _status; + + /// + /// Gets the HTTP status code of the response. Returns. 0 if response was not received. + /// + public int Status + { + get => _status; + protected set => _status = value; + } + + // Constructor from Response and InnerException + public ClientRequestException(PipelineResponse response, string? message = default, Exception? innerException = default) + : base(GetMessage(message, response), innerException) + { + _response = response; + _status = response.Status; + } + + // Constructor for case with no Response + public ClientRequestException(string message, Exception? innerException = default) + : base(message, innerException) + { + _status = 0; + } + + /// + /// TBD + /// + /// + /// + protected ClientRequestException(SerializationInfo info, StreamingContext context) + : base(info, context) + { + _status = info.GetInt32(nameof(Status)); + } + + /// + public override void GetObjectData(SerializationInfo info, StreamingContext context) + { + ClientUtilities.AssertNotNull(info, nameof(info)); + + info.AddValue(nameof(Status), Status); + + base.GetObjectData(info, context); + } + + public PipelineResponse? GetRawResponse() => _response; + + // Create message from Response if available, and override message, if available. + private static string GetMessage(string? message, PipelineResponse? response) + { + // Setting the message will override extracting it from the response. + if (message is not null) + { + return message; + } + + if (response is null) + { + return DefaultMessage; + } + + ResponseBufferingPolicy.BufferContent(response, ResponseBufferingPolicy.DefaultNetworkTimeout, new CancellationTokenSource()); + + StringBuilder messageBuilder = new(); + + messageBuilder + .AppendLine(DefaultMessage) + .Append("Status: ") + .Append(response.Status.ToString(CultureInfo.InvariantCulture)); + + if (!string.IsNullOrEmpty(response.ReasonPhrase)) + { + messageBuilder.Append(" (") + .Append(response.ReasonPhrase) + .AppendLine(")"); + } + else + { + messageBuilder.AppendLine(); + } + + // Content or headers can be obtained from raw response so are not added here. + + return messageBuilder.ToString(); + } +} diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs new file mode 100644 index 000000000000..34dac361cc2e --- /dev/null +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs @@ -0,0 +1,72 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel.Internal; +using System.ClientModel.Primitives; + +namespace System.ClientModel; + +public abstract class ClientResult +{ + private readonly PipelineResponse _response; + + protected ClientResult(PipelineResponse response) + { + ClientUtilities.AssertNotNull(response, nameof(response)); + + _response = response; + } + + /// + /// Returns the HTTP response returned by the service. + /// + /// The HTTP response returned by the service. + public PipelineResponse GetRawResponse() => _response; + + #region Factory methods for OutputMessage and subtypes + + public static ClientResult FromResponse(PipelineResponse response) + => new ClientModelClientResult(response); + + public static ClientResult FromValue(T value, PipelineResponse response) + { + // TODO: Add test to validate that the only way to create this prevents null + + // Null values must use OptionalOutputMessage + if (value is null) + { + string message = "OutputMessage contract guarantees that OutputMessage.Value is non-null. " + + "If you need to return an OutputMessage where the Value is null, please use OptionalOutputMessage instead."; + + throw new ArgumentNullException(nameof(value), message); + } + + return new ClientModelClientResult(value, response); + } + + public static OptionalClientResult FromOptionalValue(T? value, PipelineResponse response) + => new ClientModelOptionalClientResult(value, response); + + #endregion + + #region Private implementation subtypes of abstract OutputMessage types + private class ClientModelClientResult : ClientResult + { + public ClientModelClientResult(PipelineResponse response) + : base(response) { } + } + + private class ClientModelOptionalClientResult : OptionalClientResult + { + public ClientModelOptionalClientResult(T? value, PipelineResponse response) + : base(value, response) { } + } + + private class ClientModelClientResult : ClientResult + { + public ClientModelClientResult(T value, PipelineResponse response) + : base(value, response) { } + } + + #endregion +} diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs new file mode 100644 index 000000000000..b5a611f35b5d --- /dev/null +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel.Primitives; +using System.ComponentModel; + +namespace System.ClientModel; + +public abstract class ClientResult : OptionalClientResult +{ + protected ClientResult(T value, PipelineResponse response) + : base(value, response) { } + + public sealed override T Value => base.Value!; + + [EditorBrowsable(EditorBrowsableState.Never)] + public sealed override bool HasValue => true; +} diff --git a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs b/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs new file mode 100644 index 000000000000..ec69ccbb7253 --- /dev/null +++ b/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs @@ -0,0 +1,38 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Threading; + +namespace System.ClientModel; + +public class KeyCredential +{ + private string _key; + + /// + /// Initializes a new instance of the class. + /// + /// Key to use to authenticate with the Azure service. + /// + /// Thrown when the is null. + /// + /// + /// Thrown when the is empty. + /// +#pragma warning disable CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable. + public KeyCredential(string key) => Update(key); +#pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable. + + // 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); + + public void Update(string key) + { + if (key is null) throw new ArgumentNullException(nameof(key)); + if (key.Length == 0) throw new ArgumentException("Value cannot be an empty string.", nameof(key)); + + Volatile.Write(ref _key, key); + } +} diff --git a/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs new file mode 100644 index 000000000000..5cbe914c52e2 --- /dev/null +++ b/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel.Primitives; + +namespace System.ClientModel; + +public abstract class OptionalClientResult : ClientResult +{ + private readonly T? _value; + + protected OptionalClientResult(T? value, PipelineResponse response) : base(response) + => _value = value; + + /// + /// Gets the value returned by the service. Accessing this property will throw if is false. + /// + public virtual T? Value => _value; + + /// + /// Gets a value indicating whether the current instance has a valid value of its underlying type. + /// + public virtual bool HasValue => _value != null; +} diff --git a/sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs b/sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs new file mode 100644 index 000000000000..18453754a99b --- /dev/null +++ b/sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs @@ -0,0 +1,91 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Threading; +using System.Threading.Tasks; + +namespace System.ClientModel.Internal; + +internal class ClientUtilities +{ + #region Argument validation + public static void AssertNotNull(T value, string name) + { + if (value is null) + { + throw new ArgumentNullException(name); + } + } + + public static void AssertNotNullOrEmpty(string value, string name) + { + if (value is null) + { + throw new ArgumentNullException(name); + } + + if (value.Length == 0) + { + throw new ArgumentException("Value cannot be an empty string.", name); + } + } + + /// + /// Throws if is less than the or greater than the . + /// + /// The type of to validate which implements . + /// The value to validate. + /// The minimum value to compare. + /// The maximum value to compare. + /// The name of the parameter. + public static void AssertInRange(T value, T minimum, T maximum, string name) where T : notnull, IComparable + { + if (minimum.CompareTo(value) > 0) + { + throw new ArgumentOutOfRangeException(name, "Value is less than the minimum allowed."); + } + + if (maximum.CompareTo(value) < 0) + { + throw new ArgumentOutOfRangeException(name, "Value is greater than the maximum allowed."); + } + } + #endregion + + #region CancellationToken helpers + + /// The default message used by . + private static readonly string s_cancellationMessage = new OperationCanceledException().Message; // use same message as the default ctor + + /// Determines whether to wrap an in a cancellation exception. + /// The exception. + /// The that may have triggered the exception. + /// true if the exception should be wrapped; otherwise, false. + public static bool ShouldWrapInOperationCanceledException(Exception exception, CancellationToken cancellationToken) => + !(exception is OperationCanceledException) && cancellationToken.IsCancellationRequested; + + /// Throws a cancellation exception if cancellation has been requested via . + /// The token to check for a cancellation request. + public static void ThrowIfCancellationRequested(CancellationToken cancellationToken) + { + if (cancellationToken.IsCancellationRequested) + { + ThrowOperationCanceledException(innerException: null, cancellationToken); + } + } + + /// Throws a cancellation exception. + /// The inner exception to wrap. May be null. + /// The that triggered the cancellation. + private static void ThrowOperationCanceledException(Exception? innerException, CancellationToken cancellationToken) => + throw CreateOperationCanceledException(innerException, cancellationToken); + + public static Exception CreateOperationCanceledException(Exception? innerException, CancellationToken cancellationToken, string? message = null) => +#if NET6_0_OR_GREATER + new TaskCanceledException(message ?? s_cancellationMessage, innerException, cancellationToken); // TCE for compatibility with other handlers that use TaskCompletionSource.TrySetCanceled() +#else + new TaskCanceledException(message ?? s_cancellationMessage, innerException); +#endif + + #endregion +} diff --git a/sdk/core/System.ClientModel/src/Message/MessageHeaders.cs b/sdk/core/System.ClientModel/src/Message/MessageHeaders.cs new file mode 100644 index 000000000000..626f9f10da79 --- /dev/null +++ b/sdk/core/System.ClientModel/src/Message/MessageHeaders.cs @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Collections; +using System.Collections.Generic; + +namespace System.ClientModel.Primitives; + +public abstract class MessageHeaders : IEnumerable> +{ + public abstract void Add(string name, string value); + + public abstract void Set(string name, string value); + + public abstract bool Remove(string name); + + public abstract bool TryGetValue(string name, out string? value); + + public abstract bool TryGetValues(string name, out IEnumerable? values); + + public abstract IEnumerator> GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); +} diff --git a/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs b/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs new file mode 100644 index 000000000000..90699b3193d7 --- /dev/null +++ b/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs @@ -0,0 +1,89 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.IO; + +namespace System.ClientModel.Primitives; + +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()); + + private bool _isError = false; + + /// + /// Gets the HTTP status code. + /// + public abstract int Status { get; } + + /// + /// Gets the HTTP reason phrase. + /// + public abstract string ReasonPhrase { get; } + + public MessageHeaders Headers => GetHeadersCore(); + + protected abstract MessageHeaders GetHeadersCore(); + + /// + /// Gets the contents of HTTP response. Returns null for responses without content. + /// + public abstract Stream? ContentStream { get; set; } + + #region Meta-data properties set by the pipeline. + + public BinaryData Content + { + get + { + if (ContentStream == null) + { + return s_emptyBinaryData; + } + + if (!TryGetBufferedContent(out MemoryStream bufferedContent)) + { + throw new InvalidOperationException($"The response is not buffered."); + } + + if (bufferedContent.TryGetBuffer(out ArraySegment segment)) + { + return new BinaryData(segment.AsMemory()); + } + else + { + return new BinaryData(bufferedContent.ToArray()); + } + } + } + + /// + /// Indicates whether the status code of the returned response is considered + /// an error code. + /// + // IsError must be virtual in order to maintain Azure.Core back-compatibility. + public virtual bool IsError => _isError; + + // We have to have a separate method for setting IsError so that the IsError + // setter doesn't become virtual when we make the getter virtual. + internal void SetIsError(bool isError) => SetIsErrorCore(isError); + + protected virtual void SetIsErrorCore(bool isError) => _isError = isError; + + #endregion + + internal bool TryGetBufferedContent(out MemoryStream bufferedContent) + { + if (ContentStream is MemoryStream content) + { + bufferedContent = content; + return true; + } + + bufferedContent = default!; + return false; + } + + public abstract void Dispose(); +} diff --git a/sdk/core/System.ClientModel/src/System.ClientModel.csproj b/sdk/core/System.ClientModel/src/System.ClientModel.csproj index 4a3f059b18a6..7b7cef1d4b49 100644 --- a/sdk/core/System.ClientModel/src/System.ClientModel.csproj +++ b/sdk/core/System.ClientModel/src/System.ClientModel.csproj @@ -7,7 +7,8 @@ 1.0.0 enable netstandard2.0;net6.0 - $(NoWarn);AZC0001 + + $(NoWarn);AZC0001;AZC0012;AZC0014;CS1591;NETSDK1138 DotNetPackageIcon.png $(RepoEngPath)/images/$(PackageIcon) diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs new file mode 100644 index 000000000000..61b8eb0307de --- /dev/null +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs @@ -0,0 +1,113 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using ClientModel.Tests.Mocks; +using NUnit.Framework; +using System.ClientModel.Primitives; + +namespace System.ClientModel.Tests.Results; + +public class ClientResultTests +{ + #region ClientResult + + [Test] + public void CannotCreateClientResultFromNullResponse() + { + Assert.Throws(() => new MockClientResult(null!)); + Assert.Throws(() => + { + ClientResult result = ClientResult.FromResponse(null!); + }); + } + + [Test] + public void GetRawResponseReturnsResponse() + { + PipelineResponse response = new MockPipelineResponse(200, "MockReason"); + MockClientResult mockResult = new MockClientResult(response); + Assert.AreEqual(response, mockResult.GetRawResponse()); + Assert.AreEqual(response.Status, mockResult.GetRawResponse().Status); + Assert.AreEqual(response.ReasonPhrase, mockResult.GetRawResponse().ReasonPhrase); + + ClientResult result = ClientResult.FromResponse(response); + Assert.AreEqual(response, result.GetRawResponse()); + Assert.AreEqual(response.Status, result.GetRawResponse().Status); + Assert.AreEqual(response.ReasonPhrase, result.GetRawResponse().ReasonPhrase); + } + + #endregion + + #region OptionalClientResult + + [Test] + public void CannotCreateOptionalClientResultFromNullResponse() + { + Assert.Throws(() => new MockOptionalClientResult(null, null!)); + Assert.Throws(() => + { + OptionalClientResult result = ClientResult.FromOptionalValue(null, null!); + }); + } + + [Test] + public void CanCreateOptionalResultFromBool() + { + // This tests simulates creation of the result returned from a HEAD request. + + PipelineResponse response = new MockPipelineResponse(200); + OptionalClientResult result = ClientResult.FromOptionalValue(true, response); + + Assert.IsTrue(result.Value); + Assert.IsTrue(result.HasValue); + Assert.AreEqual(response.Status, result.GetRawResponse().Status); + + response = new MockPipelineResponse(400); + result = ClientResult.FromOptionalValue(false, response); + + Assert.IsFalse(result.Value); + Assert.IsTrue(result.HasValue); + Assert.AreEqual(response.Status, result.GetRawResponse().Status); + } + + [Test] + public void OptionalResultDerivedTypeCanShadowValue() + { + // This tests simulates creation of the result returned from a HEAD request. + + PipelineResponse response = new MockPipelineResponse(200); + MockPersistableModel model = new MockPersistableModel(1, "a"); + MockOptionalClientResult result = new MockOptionalClientResult(model, response); + + Assert.AreEqual(model.IntValue, result.Value!.IntValue); + Assert.AreEqual(model.StringValue, result.Value!.StringValue); + + model = new MockPersistableModel(2, "b"); + + result.SetValue(model); + + Assert.AreEqual(model.IntValue, result.Value!.IntValue); + Assert.AreEqual(model.StringValue, result.Value!.StringValue); + } + + [Test] + public void CanCreateDerivedOptionalResult() + { + // This tests simulates creation of the result returned from a HEAD request. + + PipelineResponse response = new MockPipelineResponse(500); + OptionalClientResult result = new MockErrorResult(response, new ClientRequestException(response)); + + Assert.Throws(() => { bool b = result.Value; }); + Assert.IsFalse(result.HasValue); + Assert.AreEqual(response.Status, result.GetRawResponse().Status); + } + #endregion + + #region ClientResult + + #endregion + + #region Helpers + #endregion +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResult.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResult.cs new file mode 100644 index 000000000000..deb15c965808 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResult.cs @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel; +using System.ClientModel.Primitives; + +namespace ClientModel.Tests.Mocks; + +public class MockClientResult : ClientResult +{ + public MockClientResult(PipelineResponse response) : base(response) + { + } +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs new file mode 100644 index 000000000000..f64cc9ea6577 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel; +using System.ClientModel.Primitives; + +namespace ClientModel.Tests.Mocks; + +public class MockErrorResult : OptionalClientResult +{ + private readonly ClientRequestException _exception; + + public MockErrorResult(PipelineResponse response, ClientRequestException exception) + : base(default, response) + { + _exception = exception; + } + + public override T? Value { get => throw _exception; } + + public override bool HasValue => false; +} \ No newline at end of file diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockMessageHeaders.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockMessageHeaders.cs new file mode 100644 index 000000000000..f08b0516f006 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockMessageHeaders.cs @@ -0,0 +1,41 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.ClientModel.Primitives; +using System.Collections.Generic; + +namespace ClientModel.Tests.Mocks; + +public class MockMessageHeaders : MessageHeaders +{ + public override void Add(string name, string value) + { + throw new NotImplementedException(); + } + + public override IEnumerator> GetEnumerator() + { + throw new NotImplementedException(); + } + + public override bool Remove(string name) + { + throw new NotImplementedException(); + } + + public override void Set(string name, string value) + { + throw new NotImplementedException(); + } + + public override bool TryGetValue(string name, out string? value) + { + throw new NotImplementedException(); + } + + public override bool TryGetValues(string name, out IEnumerable? values) + { + throw new NotImplementedException(); + } +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResult.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResult.cs new file mode 100644 index 000000000000..7e6c99de4fe6 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResult.cs @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel; +using System.ClientModel.Primitives; + +namespace ClientModel.Tests.Mocks; + +public class MockOptionalClientResult : OptionalClientResult +{ + private T? _value; + private bool _hasValue; + + public MockOptionalClientResult(T? value, PipelineResponse response) + : base(value, response) + { + _value = value; + } + + public override T? Value => _value; + + public void SetValue(T? value) => _value = value; + + public override bool HasValue => _hasValue; + + public void SetHasValue(bool value) => _hasValue = value; +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPersistableModel.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPersistableModel.cs new file mode 100644 index 000000000000..76ee5607d80a --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPersistableModel.cs @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using Azure; +using Azure.Core.Serialization; +using System; +using System.ClientModel.Primitives; + +namespace ClientModel.Tests.Mocks; + +public class MockPersistableModel : IPersistableModel +{ + public int IntValue { get; set; } + + public string StringValue { get; set; } + + public MockPersistableModel(int intValue, string stringValue) + { + IntValue = intValue; + StringValue = stringValue; + } + + MockPersistableModel IPersistableModel.Create(BinaryData data, ModelReaderWriterOptions options) + { + dynamic json = data.ToDynamicFromJson(JsonPropertyNames.CamelCase); + return new MockPersistableModel(json.IntValue, json.StringValue); + } + + string IPersistableModel.GetFormatFromOptions(ModelReaderWriterOptions options) + => "J"; + + BinaryData IPersistableModel.Write(ModelReaderWriterOptions options) + { + dynamic json = BinaryData.FromString("{}").ToDynamicFromJson(JsonPropertyNames.CamelCase); + json.IntValue = IntValue; + json.StringValue = StringValue; + return BinaryData.FromString(json.ToString()); + } +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs new file mode 100644 index 000000000000..40c6ef5e7650 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs @@ -0,0 +1,64 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.ClientModel.Primitives; +using System.IO; + +namespace ClientModel.Tests.Mocks; + +public class MockPipelineResponse : PipelineResponse +{ + private int _status; + private string _reasonPhrase; + private Stream? _contentStream; + + private readonly MessageHeaders _headers; + + private bool _disposed; + + public MockPipelineResponse(int status = 0, string reasonPhrase = "") + { + _status = status; + _reasonPhrase = reasonPhrase; + _headers = new MockMessageHeaders(); + } + + public override int Status => _status; + + public void SetStatus(int value) => _status = value; + + public override string ReasonPhrase => _reasonPhrase; + + public void SetReasonPhrase(string value) => _reasonPhrase = value; + + public override Stream? ContentStream + { + get => _contentStream; + set => _contentStream = value; + } + + protected override MessageHeaders GetHeadersCore() => _headers; + + public sealed override void Dispose() + { + Dispose(true); + + GC.SuppressFinalize(this); + } + + protected void Dispose(bool disposing) + { + if (disposing && !_disposed) + { + var content = _contentStream; + if (content != null) + { + _contentStream = null; + content.Dispose(); + } + + _disposed = true; + } + } +} From 56d44803dbaf0c1fd0537c5df49b1a801b707b2e Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 09:03:07 -0800 Subject: [PATCH 02/17] Change response buffering so it doesn't depend on policy --- .../src/Convenience/ClientRequestException.cs | 24 ++++++++++++++++++- .../tests/System.ClientModel.Tests.csproj | 2 ++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs index da92deb1a819..86c5833e2a69 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs @@ -4,6 +4,7 @@ using System.ClientModel.Internal; using System.ClientModel.Primitives; using System.Globalization; +using System.IO; using System.Runtime.Serialization; using System.Text; using System.Threading; @@ -79,7 +80,10 @@ private static string GetMessage(string? message, PipelineResponse? response) return DefaultMessage; } - ResponseBufferingPolicy.BufferContent(response, ResponseBufferingPolicy.DefaultNetworkTimeout, new CancellationTokenSource()); + if (!response.TryGetBufferedContent(out _)) + { + BufferResponse(response); + } StringBuilder messageBuilder = new(); @@ -103,4 +107,22 @@ private static string GetMessage(string? message, PipelineResponse? response) return messageBuilder.ToString(); } + + private static void BufferResponse(PipelineResponse response) + { + if (response.ContentStream is null) + { + return; + } + + var bufferedStream = new MemoryStream(); + response.ContentStream.CopyTo(bufferedStream); + + // Dispose the unbuffered stream + response.ContentStream.Dispose(); + + // Reset the position of the buffered stream and set it on the response + bufferedStream.Position = 0; + response.ContentStream = bufferedStream; + } } diff --git a/sdk/core/System.ClientModel/tests/System.ClientModel.Tests.csproj b/sdk/core/System.ClientModel/tests/System.ClientModel.Tests.csproj index 9dec5edf4863..dcde6363daa2 100644 --- a/sdk/core/System.ClientModel/tests/System.ClientModel.Tests.csproj +++ b/sdk/core/System.ClientModel/tests/System.ClientModel.Tests.csproj @@ -8,6 +8,7 @@ + @@ -15,6 +16,7 @@ + From 85f234df1652415d9ed5bac23f303da2c76f56ef Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 09:10:24 -0800 Subject: [PATCH 03/17] Updates/naming --- sdk/core/System.ClientModel/CHANGELOG.md | 2 +- sdk/core/System.ClientModel/README.md | 22 ++----------------- .../src/Convenience/ClientResult.cs | 12 +++++----- .../src/System.ClientModel.csproj | 2 +- 4 files changed, 9 insertions(+), 29 deletions(-) diff --git a/sdk/core/System.ClientModel/CHANGELOG.md b/sdk/core/System.ClientModel/CHANGELOG.md index 8458be1cbe50..2ff79be11660 100644 --- a/sdk/core/System.ClientModel/CHANGELOG.md +++ b/sdk/core/System.ClientModel/CHANGELOG.md @@ -4,7 +4,7 @@ ### Features Added -- Initial release of client building block types, including Pipeline, Message, and Options types. +- Initial release of convenience types in the System.ClientModel namespace, including `ClientResult`, `KeyCredential`, and `ClientRequestException`. ### Breaking Changes diff --git a/sdk/core/System.ClientModel/README.md b/sdk/core/System.ClientModel/README.md index af5938497f7e..ee5ad2bab759 100644 --- a/sdk/core/System.ClientModel/README.md +++ b/sdk/core/System.ClientModel/README.md @@ -16,7 +16,7 @@ it will be installed for you when you install one of the client libraries using Install the client library for .NET with [NuGet](https://www.nuget.org/packages/System.ClientModel). ```dotnetcli -dotnet add package System.ClientModel --prerelease +dotnet add package System.ClientModel ``` ### Prerequisites @@ -31,30 +31,12 @@ The `System.ClientModel` preview package provides a `KeyCredential` type for aut The main shared concepts of `System.ClientModel` include: -- Configuring service clients (`RequestOptions`). -- Accessing HTTP response details (`OutputMessage`, `OutputMessage`). +- Accessing HTTP response details (`ClientResult`, `ClientResult`). - Exceptions for reporting errors from service requests in a consistent fashion (`ClientRequestException`). - Providing APIs to read and write models in different formats. ## Examples -### Send a message using the MessagePipeline - -A rudimentary client implementation is as follows: - -```csharp -KeyCredential credential = new KeyCredential(key); -MessagePipeline pipeline = MessagePipeline.Create(options, new KeyCredentialAuthenticationPolicy(credential, "Authorization", "Bearer")); -ClientMessage message = pipeline.CreateMessage(options, new ResponseStatusClassifier(stackalloc ushort[] { 200 })); -MessageRequest request = message.Request; -request.SetMethod("POST"); -var uri = new RequestUri(); -uri.Reset(new Uri("https://www.example.com/")); -request.Uri = uri.ToUri(); -pipeline.Send(message); -Console.WriteLine(message.Response.Status); -``` - ### Read and write persistable models As a library author you can implement `IPersistableModel` or `IJsonModel` which will give library users the ability to read and write your models. diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs index 34dac361cc2e..334556accb53 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs @@ -23,20 +23,18 @@ protected ClientResult(PipelineResponse response) /// The HTTP response returned by the service. public PipelineResponse GetRawResponse() => _response; - #region Factory methods for OutputMessage and subtypes + #region Factory methods for ClientResult and subtypes public static ClientResult FromResponse(PipelineResponse response) => new ClientModelClientResult(response); public static ClientResult FromValue(T value, PipelineResponse response) { - // TODO: Add test to validate that the only way to create this prevents null - - // Null values must use OptionalOutputMessage + // Null values must use OptionalClientResult if (value is null) { - string message = "OutputMessage contract guarantees that OutputMessage.Value is non-null. " + - "If you need to return an OutputMessage where the Value is null, please use OptionalOutputMessage instead."; + string message = "ClientResult contract guarantees that ClientResult.Value is non-null. " + + "If you need to return an ClientResult where the Value is null, please use OptionalClientResult instead."; throw new ArgumentNullException(nameof(value), message); } @@ -49,7 +47,7 @@ public static OptionalClientResult FromOptionalValue(T? value, PipelineRes #endregion - #region Private implementation subtypes of abstract OutputMessage types + #region Private implementation subtypes of abstract ClientResult types private class ClientModelClientResult : ClientResult { public ClientModelClientResult(PipelineResponse response) diff --git a/sdk/core/System.ClientModel/src/System.ClientModel.csproj b/sdk/core/System.ClientModel/src/System.ClientModel.csproj index 7b7cef1d4b49..ca58dd585085 100644 --- a/sdk/core/System.ClientModel/src/System.ClientModel.csproj +++ b/sdk/core/System.ClientModel/src/System.ClientModel.csproj @@ -7,7 +7,7 @@ 1.0.0 enable netstandard2.0;net6.0 - + $(NoWarn);AZC0001;AZC0012;AZC0014;CS1591;NETSDK1138 DotNetPackageIcon.png $(RepoEngPath)/images/$(PackageIcon) From 5655d1a7c6b052fa87274699de8caecd0cfcd9e0 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 09:25:08 -0800 Subject: [PATCH 04/17] Add result tests --- .../src/Convenience/ClientRequestException.cs | 1 - .../ClientRequestExceptionTests.cs | 17 +++++++ .../tests/Convenience/ClientResultTests.cs | 46 +++++++++++++++++++ .../Mocks/MockClientResultOfT.cs | 14 ++++++ ...sult.cs => MockOptionalClientResultOfT.cs} | 0 5 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs create mode 100644 sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResultOfT.cs rename sdk/core/System.ClientModel/tests/TestFramework/Mocks/{MockOptionalClientResult.cs => MockOptionalClientResultOfT.cs} (100%) diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs index 86c5833e2a69..47fc66cdac08 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs @@ -7,7 +7,6 @@ using System.IO; using System.Runtime.Serialization; using System.Text; -using System.Threading; namespace System.ClientModel; diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs new file mode 100644 index 000000000000..42ff73499ef1 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs @@ -0,0 +1,17 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using ClientModel.Tests.Mocks; +using NUnit.Framework; +using System.ClientModel.Primitives; + +namespace System.ClientModel.Tests.Exceptions; + +public class ClientRequestExceptionTests +{ + //[Test] + //public void CanCreateFromResponse() + //{ + + //} +} diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs index 61b8eb0307de..b2a641355d76 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs @@ -102,12 +102,58 @@ public void CanCreateDerivedOptionalResult() Assert.IsFalse(result.HasValue); Assert.AreEqual(response.Status, result.GetRawResponse().Status); } + #endregion #region ClientResult + [Test] + public void CannotCreateClientResultOfTFromNullResponse() + { + object value = new(); + + Assert.Throws(() => new MockClientResult(value, null!)); + Assert.Throws(() => + { + OptionalClientResult result = ClientResult.FromValue(value, null!); + }); + } + + [Test] + public void CannotCreateClientResultOfTFromNullValue() + { + MockPipelineResponse response = new MockPipelineResponse(); + + Assert.Throws(() => new MockClientResult(null!, response)); + Assert.Throws(() => + { + OptionalClientResult result = ClientResult.FromValue(null!, response); + }); + } + + [Test] + public void CanCreateDerivedResult() + { + string value = "hello"; + + PipelineResponse response = new MockPipelineResponse(200); + DerivedClientResult result = new(value, response); + + Assert.IsTrue(result.HasValue); + Assert.AreEqual(value, result.Value); + Assert.AreEqual(response.Status, result.GetRawResponse().Status); + } + #endregion #region Helpers + + internal class DerivedClientResult : ClientResult + { + public DerivedClientResult(T value, PipelineResponse response) : base(value, response) + { + } + } + #endregion } diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResultOfT.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResultOfT.cs new file mode 100644 index 000000000000..199b1772969c --- /dev/null +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockClientResultOfT.cs @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.ClientModel; +using System.ClientModel.Primitives; + +namespace ClientModel.Tests.Mocks; + +public class MockClientResult : ClientResult +{ + public MockClientResult(T value, PipelineResponse response) : base(value, response) + { + } +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResult.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResultOfT.cs similarity index 100% rename from sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResult.cs rename to sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockOptionalClientResultOfT.cs From e8bc4185c0ffb88034b952c343672a0769ddb4ea Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 09:26:27 -0800 Subject: [PATCH 05/17] Export API --- .../api/System.ClientModel.net6.0.cs | 63 +++++++++++++++++++ .../api/System.ClientModel.netstandard2.0.cs | 63 +++++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs index 953f641fb969..659e611173d1 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs @@ -1,3 +1,42 @@ +namespace System.ClientModel +{ + public partial class ClientRequestException : System.Exception, System.Runtime.Serialization.ISerializable + { + public ClientRequestException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } + protected ClientRequestException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public ClientRequestException(string message, System.Exception? innerException = null) { } + public int Status { get { throw null; } protected set { } } + public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } + } + public abstract partial class ClientResult + { + protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } + public static System.ClientModel.OptionalClientResult FromOptionalValue(T? value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } + public static System.ClientModel.ClientResult FromResponse(System.ClientModel.Primitives.PipelineResponse response) { throw null; } + public static System.ClientModel.ClientResult FromValue(T value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } + public System.ClientModel.Primitives.PipelineResponse GetRawResponse() { throw null; } + } + public abstract partial class ClientResult : System.ClientModel.OptionalClientResult + { + protected ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] + public sealed override bool HasValue { get { throw null; } } + public sealed override T Value { get { throw null; } } + } + public partial class KeyCredential + { + public KeyCredential(string key) { } + public string GetValue() { throw null; } + public void Update(string key) { } + } + public abstract partial class OptionalClientResult : System.ClientModel.ClientResult + { + protected OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } + public virtual bool HasValue { get { throw null; } } + public virtual T? Value { get { throw null; } } + } +} namespace System.ClientModel.Primitives { public partial interface IJsonModel : System.ClientModel.Primitives.IPersistableModel @@ -11,6 +50,17 @@ public partial interface IPersistableModel string GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options); System.BinaryData Write(System.ClientModel.Primitives.ModelReaderWriterOptions options); } + public abstract partial class MessageHeaders : System.Collections.Generic.IEnumerable>, System.Collections.IEnumerable + { + protected MessageHeaders() { } + public abstract void Add(string name, string value); + public abstract System.Collections.Generic.IEnumerator> GetEnumerator(); + public abstract bool Remove(string name); + public abstract void Set(string name, string value); + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } + public abstract bool TryGetValue(string name, out string? value); + public abstract bool TryGetValues(string name, out System.Collections.Generic.IEnumerable? values); + } public static partial class ModelReaderWriter { public static object? Read(System.BinaryData data, [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicConstructors | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] System.Type returnType, System.ClientModel.Primitives.ModelReaderWriterOptions? options = null) { throw null; } @@ -32,4 +82,17 @@ public PersistableModelProxyAttribute([System.Diagnostics.CodeAnalysis.Dynamical [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembersAttribute(System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicConstructors | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors)] public System.Type ProxyType { get { throw null; } } } + public abstract partial class PipelineResponse : System.IDisposable + { + protected PipelineResponse() { } + public System.BinaryData Content { get { throw null; } } + public abstract System.IO.Stream? ContentStream { get; set; } + public System.ClientModel.Primitives.MessageHeaders Headers { get { throw null; } } + public virtual bool IsError { get { throw null; } } + public abstract string ReasonPhrase { get; } + public abstract int Status { get; } + public abstract void Dispose(); + protected abstract System.ClientModel.Primitives.MessageHeaders GetHeadersCore(); + protected virtual void SetIsErrorCore(bool isError) { } + } } diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs index 39d84a0354fe..d7b0fdda8a94 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs @@ -1,3 +1,42 @@ +namespace System.ClientModel +{ + public partial class ClientRequestException : System.Exception, System.Runtime.Serialization.ISerializable + { + public ClientRequestException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } + protected ClientRequestException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public ClientRequestException(string message, System.Exception? innerException = null) { } + public int Status { get { throw null; } protected set { } } + public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } + } + public abstract partial class ClientResult + { + protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } + public static System.ClientModel.OptionalClientResult FromOptionalValue(T? value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } + public static System.ClientModel.ClientResult FromResponse(System.ClientModel.Primitives.PipelineResponse response) { throw null; } + public static System.ClientModel.ClientResult FromValue(T value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } + public System.ClientModel.Primitives.PipelineResponse GetRawResponse() { throw null; } + } + public abstract partial class ClientResult : System.ClientModel.OptionalClientResult + { + protected ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] + public sealed override bool HasValue { get { throw null; } } + public sealed override T Value { get { throw null; } } + } + public partial class KeyCredential + { + public KeyCredential(string key) { } + public string GetValue() { throw null; } + public void Update(string key) { } + } + public abstract partial class OptionalClientResult : System.ClientModel.ClientResult + { + protected OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } + public virtual bool HasValue { get { throw null; } } + public virtual T? Value { get { throw null; } } + } +} namespace System.ClientModel.Primitives { public partial interface IJsonModel : System.ClientModel.Primitives.IPersistableModel @@ -11,6 +50,17 @@ public partial interface IPersistableModel string GetFormatFromOptions(System.ClientModel.Primitives.ModelReaderWriterOptions options); System.BinaryData Write(System.ClientModel.Primitives.ModelReaderWriterOptions options); } + public abstract partial class MessageHeaders : System.Collections.Generic.IEnumerable>, System.Collections.IEnumerable + { + protected MessageHeaders() { } + public abstract void Add(string name, string value); + public abstract System.Collections.Generic.IEnumerator> GetEnumerator(); + public abstract bool Remove(string name); + public abstract void Set(string name, string value); + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; } + public abstract bool TryGetValue(string name, out string? value); + public abstract bool TryGetValues(string name, out System.Collections.Generic.IEnumerable? values); + } public static partial class ModelReaderWriter { public static object? Read(System.BinaryData data, System.Type returnType, System.ClientModel.Primitives.ModelReaderWriterOptions? options = null) { throw null; } @@ -31,4 +81,17 @@ public sealed partial class PersistableModelProxyAttribute : System.Attribute public PersistableModelProxyAttribute(System.Type proxyType) { } public System.Type ProxyType { get { throw null; } } } + public abstract partial class PipelineResponse : System.IDisposable + { + protected PipelineResponse() { } + public System.BinaryData Content { get { throw null; } } + public abstract System.IO.Stream? ContentStream { get; set; } + public System.ClientModel.Primitives.MessageHeaders Headers { get { throw null; } } + public virtual bool IsError { get { throw null; } } + public abstract string ReasonPhrase { get; } + public abstract int Status { get; } + public abstract void Dispose(); + protected abstract System.ClientModel.Primitives.MessageHeaders GetHeadersCore(); + protected virtual void SetIsErrorCore(bool isError) { } + } } From 511553dea60026fa99636114a2b79eec6d7bb3a1 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 09:58:03 -0800 Subject: [PATCH 06/17] test fix --- .../src/Convenience/ClientResultOfT.cs | 6 +++++- .../tests/Convenience/ClientResultTests.cs | 12 ++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs index b5a611f35b5d..555aa27a5677 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System.ClientModel.Internal; using System.ClientModel.Primitives; using System.ComponentModel; @@ -9,7 +10,10 @@ namespace System.ClientModel; public abstract class ClientResult : OptionalClientResult { protected ClientResult(T value, PipelineResponse response) - : base(value, response) { } + : base(value, response) + { + ClientUtilities.AssertNotNull(value, nameof(value)); + } public sealed override T Value => base.Value!; diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs index b2a641355d76..bb58d57daa8b 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs @@ -51,7 +51,7 @@ public void CannotCreateOptionalClientResultFromNullResponse() } [Test] - public void CanCreateOptionalResultFromBool() + public void CanCreateOptionalClientResultFromBool() { // This tests simulates creation of the result returned from a HEAD request. @@ -71,7 +71,7 @@ public void CanCreateOptionalResultFromBool() } [Test] - public void OptionalResultDerivedTypeCanShadowValue() + public void OptionalClientResultDerivedTypeCanShadowValue() { // This tests simulates creation of the result returned from a HEAD request. @@ -91,7 +91,7 @@ public void OptionalResultDerivedTypeCanShadowValue() } [Test] - public void CanCreateDerivedOptionalResult() + public void CanCreateDerivedOptionalClientResult() { // This tests simulates creation of the result returned from a HEAD request. @@ -115,7 +115,7 @@ public void CannotCreateClientResultOfTFromNullResponse() Assert.Throws(() => new MockClientResult(value, null!)); Assert.Throws(() => { - OptionalClientResult result = ClientResult.FromValue(value, null!); + ClientResult result = ClientResult.FromValue(value, null!); }); } @@ -127,12 +127,12 @@ public void CannotCreateClientResultOfTFromNullValue() Assert.Throws(() => new MockClientResult(null!, response)); Assert.Throws(() => { - OptionalClientResult result = ClientResult.FromValue(null!, response); + ClientResult result = ClientResult.FromValue(null!, response); }); } [Test] - public void CanCreateDerivedResult() + public void CanCreateDerivedClientResultOfT() { string value = "hello"; From 65e949d57ae0bfc79feef1399cff68883950c17a Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 10:09:45 -0800 Subject: [PATCH 07/17] Add tests for Exception --- .../src/Convenience/ClientResultOfT.cs | 1 + .../ClientRequestExceptionTests.cs | 50 +++++++++++++++++-- 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs index 555aa27a5677..ccaf2dedd931 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs @@ -12,6 +12,7 @@ public abstract class ClientResult : OptionalClientResult protected ClientResult(T value, PipelineResponse response) : base(value, response) { + // Null values must use OptionalClientResult ClientUtilities.AssertNotNull(value, nameof(value)); } diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs index 42ff73499ef1..893fa20754db 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs @@ -9,9 +9,51 @@ namespace System.ClientModel.Tests.Exceptions; public class ClientRequestExceptionTests { - //[Test] - //public void CanCreateFromResponse() - //{ + [Test] + public void CanCreateFromResponse() + { + PipelineResponse response = new MockPipelineResponse(200, "MockReason"); - //} + ClientRequestException exception = new ClientRequestException(response); + + Assert.AreEqual(response.Status, exception.Status); + Assert.AreEqual(response, exception.GetRawResponse()); + Assert.AreEqual("Service request failed.\r\nStatus: 200 (MockReason)\r\n", exception.Message); + } + + [Test] + public void PassingMessageOverridesResponseMessage() + { + PipelineResponse response = new MockPipelineResponse(200, "MockReason"); + string message = "Override Message"; + + ClientRequestException exception = new ClientRequestException(response, message); + + Assert.AreEqual(response.Status, exception.Status); + Assert.AreEqual(response, exception.GetRawResponse()); + Assert.AreEqual(message, exception.Message); + } + + [Test] + public void UsesDefaultMessageWhenResponseIsNull() + { + PipelineResponse? response = null; + ClientRequestException exception = new ClientRequestException(response!); + + Assert.AreEqual(0, exception.Status); + Assert.IsNull(exception.GetRawResponse()); + Assert.AreEqual("Service request failed.", exception.Message); + } + + [Test] + public void CanCreateFromMessage() + { + string message = "Override Message"; + + ClientRequestException exception = new ClientRequestException(message); + + Assert.AreEqual(0, exception.Status); + Assert.IsNull(exception.GetRawResponse()); + Assert.AreEqual(message, exception.Message); + } } From c54f476ab32b67c8dcb51b2e57ea803c97996974 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 10:37:53 -0800 Subject: [PATCH 08/17] buffering test --- .../src/Convenience/ClientRequestException.cs | 15 ++-- .../ClientRequestExceptionTests.cs | 87 ++++++++++++++++--- 2 files changed, 82 insertions(+), 20 deletions(-) diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs index 47fc66cdac08..eb61a905d23f 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs @@ -27,15 +27,15 @@ public int Status protected set => _status = value; } - // Constructor from Response and InnerException public ClientRequestException(PipelineResponse response, string? message = default, Exception? innerException = default) - : base(GetMessage(message, response), innerException) + : base(GetMessage(response, message), innerException) { + ClientUtilities.AssertNotNull(response, nameof(response)); + _response = response; _status = response.Status; } - // Constructor for case with no Response public ClientRequestException(string message, Exception? innerException = default) : base(message, innerException) { @@ -65,8 +65,8 @@ public override void GetObjectData(SerializationInfo info, StreamingContext cont public PipelineResponse? GetRawResponse() => _response; - // Create message from Response if available, and override message, if available. - private static string GetMessage(string? message, PipelineResponse? response) + // Create message from response if available, and override message, if available. + private static string GetMessage(PipelineResponse response, string? message) { // Setting the message will override extracting it from the response. if (message is not null) @@ -74,11 +74,6 @@ private static string GetMessage(string? message, PipelineResponse? response) return message; } - if (response is null) - { - return DefaultMessage; - } - if (!response.TryGetBufferedContent(out _)) { BufferResponse(response); diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs index 893fa20754db..e5fc946d81f6 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs @@ -4,6 +4,7 @@ using ClientModel.Tests.Mocks; using NUnit.Framework; using System.ClientModel.Primitives; +using System.IO; namespace System.ClientModel.Tests.Exceptions; @@ -35,25 +36,91 @@ public void PassingMessageOverridesResponseMessage() } [Test] - public void UsesDefaultMessageWhenResponseIsNull() + public void CanCreateFromMessage() { - PipelineResponse? response = null; - ClientRequestException exception = new ClientRequestException(response!); + string message = "Override Message"; + + ClientRequestException exception = new ClientRequestException(message); Assert.AreEqual(0, exception.Status); Assert.IsNull(exception.GetRawResponse()); - Assert.AreEqual("Service request failed.", exception.Message); + Assert.AreEqual(message, exception.Message); } [Test] - public void CanCreateFromMessage() + public void UnbufferedResponseIsBuffered() { - string message = "Override Message"; + byte[] content = new byte[] { 0 }; - ClientRequestException exception = new ClientRequestException(message); + PipelineResponse response = new MockPipelineResponse(200, "MockReason"); + response.ContentStream = new UnbufferedStream(content); - Assert.AreEqual(0, exception.Status); - Assert.IsNull(exception.GetRawResponse()); - Assert.AreEqual(message, exception.Message); + ClientRequestException exception = new ClientRequestException(response); + + Assert.AreEqual(response.Status, exception.Status); + Assert.AreEqual(response, exception.GetRawResponse()); + + // Accessing Content would throw if it hadn't been buffered. + Assert.AreEqual(content, response.Content.ToArray()); } + + #region + + internal class UnbufferedStream : Stream + { + private readonly byte[] _bytes; + private long _position; + + public UnbufferedStream(byte[] bytes) + { + _bytes = bytes; + _position = 0; + } + + public override bool CanRead => true; + + public override bool CanSeek => true; + + public override bool CanWrite => false; + + public override long Length => _bytes.Length; + + public override long Position + { + get => _position; + set => _position = value; + } + + public override void Flush() { } + + public override int Read(byte[] buffer, int offset, int count) + { + if (_position >= _bytes.Length) + { + return 0; + } + + // Assumes we copy everything on the first read + Array.Copy(_bytes, offset, buffer, offset, _bytes.Length); + _position += _bytes.Length; + return _bytes.Length; + } + + public override long Seek(long offset, SeekOrigin origin) + { + return 0; + } + + public override void SetLength(long value) + { + throw new NotSupportedException(); + } + + public override void Write(byte[] buffer, int offset, int count) + { + throw new NotSupportedException(); + } + } + + #endregion } From f0ff36ffeb4b92d204144cbe514eaee6f904099f Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 10:54:03 -0800 Subject: [PATCH 09/17] more tests --- .../ClientRequestExceptionTests.cs | 4 +- .../tests/Convenience/ClientResultTests.cs | 2 +- .../tests/Message/PipelineResponseTests.cs | 86 +++++++++++++++++++ .../Mocks/MockPipelineResponse.cs | 12 +++ 4 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 sdk/core/System.ClientModel/tests/Message/PipelineResponseTests.cs diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs index e5fc946d81f6..80f61b9906e1 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs @@ -19,7 +19,9 @@ public void CanCreateFromResponse() Assert.AreEqual(response.Status, exception.Status); Assert.AreEqual(response, exception.GetRawResponse()); - Assert.AreEqual("Service request failed.\r\nStatus: 200 (MockReason)\r\n", exception.Message); + Assert.AreEqual( + $"Service request failed.{Environment.NewLine}Status: 200 (MockReason){Environment.NewLine}", + exception.Message); } [Test] diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs index bb58d57daa8b..e02cc1ea6e83 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs @@ -7,7 +7,7 @@ namespace System.ClientModel.Tests.Results; -public class ClientResultTests +public class PipelineResponseTests { #region ClientResult diff --git a/sdk/core/System.ClientModel/tests/Message/PipelineResponseTests.cs b/sdk/core/System.ClientModel/tests/Message/PipelineResponseTests.cs new file mode 100644 index 000000000000..a77a667de2b5 --- /dev/null +++ b/sdk/core/System.ClientModel/tests/Message/PipelineResponseTests.cs @@ -0,0 +1,86 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using ClientModel.Tests.Mocks; +using NUnit.Framework; +using System.IO; + +namespace System.ClientModel.Tests.Message; + +public class PipelineResponseTests +{ + [Test] + public void ContentPropertyGetsContent() + { + var response = new MockPipelineResponse(200); + Assert.AreEqual(0, response.Content.ToArray().Length); + + var responseWithBody = new MockPipelineResponse(200); + responseWithBody.SetContent("body content"); + Assert.AreEqual("body content", responseWithBody.Content.ToString()); + + // Ensure that the BinaryData is formed over the used portion of the memory stream, not the entire buffer. + MemoryStream ms = new MemoryStream(50); + var responseWithEmptyBody = new MockPipelineResponse(200); + responseWithEmptyBody.ContentStream = ms; + Assert.AreEqual(0, response.Content.ToArray().Length); + + // Ensure that even if the stream has been read and the cursor is sitting at the end of stream, the + // `Content` property still contains the entire response. + var responseWithBodyFullyRead = new MockPipelineResponse(200); + responseWithBodyFullyRead.SetContent("body content"); + responseWithBodyFullyRead.ContentStream!.Seek(0, SeekOrigin.End); + Assert.AreEqual("body content", responseWithBody.Content.ToString()); + } + + [Test] + public void ContentPropertyThrowsForNonMemoryStream() + { + var response = new MockPipelineResponse(200); + response.ContentStream = new ThrowingStream(); + + Assert.Throws(() => { BinaryData d = response.Content; }); + } + + #region Helpers + + internal class ThrowingStream : Stream + { + public override bool CanRead => throw new System.NotImplementedException(); + + public override bool CanSeek => throw new System.NotImplementedException(); + + public override bool CanWrite => throw new System.NotImplementedException(); + + public override long Length => throw new System.NotImplementedException(); + + public override long Position { get => throw new System.NotImplementedException(); set => throw new System.NotImplementedException(); } + + public override void Flush() + { + throw new System.NotImplementedException(); + } + + public override int Read(byte[] buffer, int offset, int count) + { + throw new System.NotImplementedException(); + } + + public override long Seek(long offset, SeekOrigin origin) + { + throw new System.NotImplementedException(); + } + + public override void SetLength(long value) + { + throw new System.NotImplementedException(); + } + + public override void Write(byte[] buffer, int offset, int count) + { + throw new System.NotImplementedException(); + } + } + + #endregion +} diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs index 40c6ef5e7650..9cf411ac5739 100644 --- a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs @@ -4,6 +4,7 @@ using System; using System.ClientModel.Primitives; using System.IO; +using System.Text; namespace ClientModel.Tests.Mocks; @@ -32,6 +33,17 @@ public MockPipelineResponse(int status = 0, string reasonPhrase = "") public void SetReasonPhrase(string value) => _reasonPhrase = value; + public void SetContent(byte[] content) + { + ContentStream = new MemoryStream(content, 0, content.Length, false, true); + } + + public MockPipelineResponse SetContent(string content) + { + SetContent(Encoding.UTF8.GetBytes(content)); + return this; + } + public override Stream? ContentStream { get => _contentStream; From 9e0db267525d6f60a69ba03b67b6bee348901efc Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 14:12:32 -0800 Subject: [PATCH 10/17] Rename ClientRequestException -> ClientResultException --- sdk/core/System.ClientModel/CHANGELOG.md | 2 +- sdk/core/System.ClientModel/README.md | 4 ++-- .../api/System.ClientModel.net6.0.cs | 18 +++++++++--------- .../api/System.ClientModel.netstandard2.0.cs | 18 +++++++++--------- ...stException.cs => ClientResultException.cs} | 8 ++++---- .../Convenience/ClientRequestExceptionTests.cs | 10 +++++----- .../tests/Convenience/ClientResultTests.cs | 4 ++-- .../TestFramework/Mocks/MockErrorResult.cs | 4 ++-- 8 files changed, 34 insertions(+), 34 deletions(-) rename sdk/core/System.ClientModel/src/Convenience/{ClientRequestException.cs => ClientResultException.cs} (89%) diff --git a/sdk/core/System.ClientModel/CHANGELOG.md b/sdk/core/System.ClientModel/CHANGELOG.md index 2ff79be11660..6e3bd5810fb2 100644 --- a/sdk/core/System.ClientModel/CHANGELOG.md +++ b/sdk/core/System.ClientModel/CHANGELOG.md @@ -4,7 +4,7 @@ ### Features Added -- Initial release of convenience types in the System.ClientModel namespace, including `ClientResult`, `KeyCredential`, and `ClientRequestException`. +- Initial release of convenience types in the System.ClientModel namespace, including `ClientResult`, `KeyCredential`, and `ClientResultException`. ### Breaking Changes diff --git a/sdk/core/System.ClientModel/README.md b/sdk/core/System.ClientModel/README.md index ee5ad2bab759..357746e2eb09 100644 --- a/sdk/core/System.ClientModel/README.md +++ b/sdk/core/System.ClientModel/README.md @@ -32,7 +32,7 @@ The `System.ClientModel` preview package provides a `KeyCredential` type for aut The main shared concepts of `System.ClientModel` include: - Accessing HTTP response details (`ClientResult`, `ClientResult`). -- Exceptions for reporting errors from service requests in a consistent fashion (`ClientRequestException`). +- Exceptions for reporting errors from service requests in a consistent fashion (`ClientResultException`). - Providing APIs to read and write models in different formats. ## Examples @@ -61,7 +61,7 @@ OutputModel? model = ModelReaderWriter.Read(BinaryData.FromString(j ## Troubleshooting -You can troubleshoot `System.ClientModel`-based clients by inspecting the result of any `ClientRequestException` thrown from a pipeline's `Send` method. +You can troubleshoot `System.ClientModel`-based clients by inspecting the result of any `ClientResultException` thrown from a pipeline's `Send` method. ## Next steps diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs index 659e611173d1..7c1a4d07cb63 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs @@ -1,14 +1,5 @@ namespace System.ClientModel { - public partial class ClientRequestException : System.Exception, System.Runtime.Serialization.ISerializable - { - public ClientRequestException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } - protected ClientRequestException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } - public ClientRequestException(string message, System.Exception? innerException = null) { } - public int Status { get { throw null; } protected set { } } - public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } - public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } - } public abstract partial class ClientResult { protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } @@ -17,6 +8,15 @@ protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) public static System.ClientModel.ClientResult FromValue(T value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } public System.ClientModel.Primitives.PipelineResponse GetRawResponse() { throw null; } } + public partial class ClientResultException : System.Exception, System.Runtime.Serialization.ISerializable + { + public ClientResultException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } + protected ClientResultException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public ClientResultException(string message, System.Exception? innerException = null) { } + public int Status { get { throw null; } protected set { } } + public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } + } public abstract partial class ClientResult : System.ClientModel.OptionalClientResult { protected ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs index d7b0fdda8a94..a5aa6e4bac08 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs @@ -1,14 +1,5 @@ namespace System.ClientModel { - public partial class ClientRequestException : System.Exception, System.Runtime.Serialization.ISerializable - { - public ClientRequestException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } - protected ClientRequestException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } - public ClientRequestException(string message, System.Exception? innerException = null) { } - public int Status { get { throw null; } protected set { } } - public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } - public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } - } public abstract partial class ClientResult { protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } @@ -17,6 +8,15 @@ protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) public static System.ClientModel.ClientResult FromValue(T value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } public System.ClientModel.Primitives.PipelineResponse GetRawResponse() { throw null; } } + public partial class ClientResultException : System.Exception, System.Runtime.Serialization.ISerializable + { + public ClientResultException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } + protected ClientResultException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public ClientResultException(string message, System.Exception? innerException = null) { } + public int Status { get { throw null; } protected set { } } + public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } + public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } + } public abstract partial class ClientResult : System.ClientModel.OptionalClientResult { protected ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs similarity index 89% rename from sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs rename to sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs index eb61a905d23f..82a8cf861c09 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientRequestException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs @@ -11,7 +11,7 @@ namespace System.ClientModel; [Serializable] -public class ClientRequestException : Exception, ISerializable +public class ClientResultException : Exception, ISerializable { private const string DefaultMessage = "Service request failed."; @@ -27,7 +27,7 @@ public int Status protected set => _status = value; } - public ClientRequestException(PipelineResponse response, string? message = default, Exception? innerException = default) + public ClientResultException(PipelineResponse response, string? message = default, Exception? innerException = default) : base(GetMessage(response, message), innerException) { ClientUtilities.AssertNotNull(response, nameof(response)); @@ -36,7 +36,7 @@ public ClientRequestException(PipelineResponse response, string? message = defau _status = response.Status; } - public ClientRequestException(string message, Exception? innerException = default) + public ClientResultException(string message, Exception? innerException = default) : base(message, innerException) { _status = 0; @@ -47,7 +47,7 @@ public ClientRequestException(string message, Exception? innerException = defaul /// /// /// - protected ClientRequestException(SerializationInfo info, StreamingContext context) + protected ClientResultException(SerializationInfo info, StreamingContext context) : base(info, context) { _status = info.GetInt32(nameof(Status)); diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs index 80f61b9906e1..176a296fe177 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs @@ -8,14 +8,14 @@ namespace System.ClientModel.Tests.Exceptions; -public class ClientRequestExceptionTests +public class ClientResultExceptionTests { [Test] public void CanCreateFromResponse() { PipelineResponse response = new MockPipelineResponse(200, "MockReason"); - ClientRequestException exception = new ClientRequestException(response); + ClientResultException exception = new ClientResultException(response); Assert.AreEqual(response.Status, exception.Status); Assert.AreEqual(response, exception.GetRawResponse()); @@ -30,7 +30,7 @@ public void PassingMessageOverridesResponseMessage() PipelineResponse response = new MockPipelineResponse(200, "MockReason"); string message = "Override Message"; - ClientRequestException exception = new ClientRequestException(response, message); + ClientResultException exception = new ClientResultException(response, message); Assert.AreEqual(response.Status, exception.Status); Assert.AreEqual(response, exception.GetRawResponse()); @@ -42,7 +42,7 @@ public void CanCreateFromMessage() { string message = "Override Message"; - ClientRequestException exception = new ClientRequestException(message); + ClientResultException exception = new ClientResultException(message); Assert.AreEqual(0, exception.Status); Assert.IsNull(exception.GetRawResponse()); @@ -57,7 +57,7 @@ public void UnbufferedResponseIsBuffered() PipelineResponse response = new MockPipelineResponse(200, "MockReason"); response.ContentStream = new UnbufferedStream(content); - ClientRequestException exception = new ClientRequestException(response); + ClientResultException exception = new ClientResultException(response); Assert.AreEqual(response.Status, exception.Status); Assert.AreEqual(response, exception.GetRawResponse()); diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs index e02cc1ea6e83..397e69a46b1f 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientResultTests.cs @@ -96,9 +96,9 @@ public void CanCreateDerivedOptionalClientResult() // This tests simulates creation of the result returned from a HEAD request. PipelineResponse response = new MockPipelineResponse(500); - OptionalClientResult result = new MockErrorResult(response, new ClientRequestException(response)); + OptionalClientResult result = new MockErrorResult(response, new ClientResultException(response)); - Assert.Throws(() => { bool b = result.Value; }); + Assert.Throws(() => { bool b = result.Value; }); Assert.IsFalse(result.HasValue); Assert.AreEqual(response.Status, result.GetRawResponse().Status); } diff --git a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs index f64cc9ea6577..ac7f081c082d 100644 --- a/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs +++ b/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockErrorResult.cs @@ -8,9 +8,9 @@ namespace ClientModel.Tests.Mocks; public class MockErrorResult : OptionalClientResult { - private readonly ClientRequestException _exception; + private readonly ClientResultException _exception; - public MockErrorResult(PipelineResponse response, ClientRequestException exception) + public MockErrorResult(PipelineResponse response, ClientResultException exception) : base(default, response) { _exception = exception; From 0ed0d5947a4a97ed372fe83309d95f9d1dfd1842 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 17:09:39 -0800 Subject: [PATCH 11/17] Move response buffering into PipelineResponse --- .../src/Convenience/ClientResultException.cs | 23 +---- .../src/Internal/StreamExtensions.cs | 89 ++++++++++++++++++ .../src/Message/PipelineResponse.cs | 92 ++++++++++++++++++- 3 files changed, 181 insertions(+), 23 deletions(-) create mode 100644 sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs index 82a8cf861c09..eeb52fabc62c 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs @@ -74,10 +74,7 @@ private static string GetMessage(PipelineResponse response, string? message) return message; } - if (!response.TryGetBufferedContent(out _)) - { - BufferResponse(response); - } + response.BufferContent(); StringBuilder messageBuilder = new(); @@ -101,22 +98,4 @@ private static string GetMessage(PipelineResponse response, string? message) return messageBuilder.ToString(); } - - private static void BufferResponse(PipelineResponse response) - { - if (response.ContentStream is null) - { - return; - } - - var bufferedStream = new MemoryStream(); - response.ContentStream.CopyTo(bufferedStream); - - // Dispose the unbuffered stream - response.ContentStream.Dispose(); - - // Reset the position of the buffered stream and set it on the response - bufferedStream.Position = 0; - response.ContentStream = bufferedStream; - } } diff --git a/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs b/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs new file mode 100644 index 000000000000..b0b70270fbd0 --- /dev/null +++ b/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs @@ -0,0 +1,89 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System.Buffers; +using System.Diagnostics; +using System.IO; +using System.Runtime.InteropServices; +using System.Threading; +using System.Threading.Tasks; + +namespace System.ClientModel.Internal; + +internal static class StreamExtensions +{ + public static async Task WriteAsync(this Stream stream, ReadOnlyMemory buffer, CancellationToken cancellation = default) + { + ClientUtilities.AssertNotNull(stream, nameof(stream)); +#if NETCOREAPP + await stream.WriteAsync(buffer, cancellation).ConfigureAwait(false); +#else + + if (buffer.Length == 0) + return; + byte[]? array = null; + try + { + if (MemoryMarshal.TryGetArray(buffer, out ArraySegment arraySegment)) + { + Debug.Assert(arraySegment.Array != null); + await stream.WriteAsync(arraySegment.Array, arraySegment.Offset, arraySegment.Count, cancellation).ConfigureAwait(false); + } + else + { + array = ArrayPool.Shared.Rent(buffer.Length); + + if (!buffer.TryCopyTo(array)) + throw new Exception("could not rent large enough buffer."); + await stream.WriteAsync(array, 0, buffer.Length, cancellation).ConfigureAwait(false); + } + } + finally + { + if (array != null) + ArrayPool.Shared.Return(array); + } +#endif + } + + public static async Task WriteAsync(this Stream stream, ReadOnlySequence buffer, CancellationToken cancellation = default) + { + ClientUtilities.AssertNotNull(stream, nameof(stream)); + + if (buffer.Length == 0) + return; + byte[]? array = null; + try + { + foreach (ReadOnlyMemory segment in buffer) + { +#if NETCOREAPP + await stream.WriteAsync(segment, cancellation).ConfigureAwait(false); +#else + if (MemoryMarshal.TryGetArray(segment, out ArraySegment arraySegment)) + { + Debug.Assert(arraySegment.Array != null); + await stream.WriteAsync(arraySegment.Array, arraySegment.Offset, arraySegment.Count, cancellation).ConfigureAwait(false); + } + else + { + if (array == null || array.Length < segment.Length) + { + if (array != null) + ArrayPool.Shared.Return(array); + array = ArrayPool.Shared.Rent(segment.Length); + } + if (!segment.TryCopyTo(array)) + throw new Exception("could not rent large enough buffer."); + await stream.WriteAsync(array, 0, segment.Length, cancellation).ConfigureAwait(false); + } +#endif + } + } + finally + { + if (array != null) + ArrayPool.Shared.Return(array); + } + } +} diff --git a/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs b/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs index 90699b3193d7..1debf1b69cef 100644 --- a/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs +++ b/sdk/core/System.ClientModel/src/Message/PipelineResponse.cs @@ -1,12 +1,19 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System.Buffers; +using System.ClientModel.Internal; using System.IO; +using System.Threading; +using System.Threading.Tasks; namespace System.ClientModel.Primitives; public abstract class PipelineResponse : IDisposable { + // TODO: We can move this onto ClientPipeline once we move that type into main. + internal static TimeSpan DefaultNetworkTimeout { get; } = TimeSpan.FromSeconds(100); + // 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()); @@ -71,8 +78,17 @@ public BinaryData Content protected virtual void SetIsErrorCore(bool isError) => _isError = isError; + internal TimeSpan NetworkTimeout { get; set; } = DefaultNetworkTimeout; + #endregion + public abstract void Dispose(); + + #region Response Buffering + + // Same value as Stream.CopyTo uses by default + private const int DefaultCopyBufferSize = 81920; + internal bool TryGetBufferedContent(out MemoryStream bufferedContent) { if (ContentStream is MemoryStream content) @@ -85,5 +101,79 @@ internal bool TryGetBufferedContent(out MemoryStream bufferedContent) return false; } - public abstract void Dispose(); + internal void BufferContent(TimeSpan? timeout = default, CancellationTokenSource? cts = default) + { + Stream? responseContentStream = ContentStream; + if (responseContentStream == null || TryGetBufferedContent(out _)) + { + // No need to buffer content. + return; + } + + MemoryStream bufferStream = new(); + CopyTo(responseContentStream, bufferStream, timeout ?? NetworkTimeout, cts ?? new CancellationTokenSource()); + responseContentStream.Dispose(); + bufferStream.Position = 0; + ContentStream = bufferStream; + } + + internal async Task BufferContentAsync(TimeSpan? timeout = default, CancellationTokenSource? cts = default) + { + Stream? responseContentStream = ContentStream; + if (responseContentStream == null || TryGetBufferedContent(out _)) + { + // No need to buffer content. + return; + } + + MemoryStream bufferStream = new(); + await CopyToAsync(responseContentStream, bufferStream, timeout ?? NetworkTimeout, cts ?? new CancellationTokenSource()).ConfigureAwait(false); + responseContentStream.Dispose(); + bufferStream.Position = 0; + ContentStream = bufferStream; + } + + private static async Task CopyToAsync(Stream source, Stream destination, TimeSpan timeout, CancellationTokenSource cancellationTokenSource) + { + byte[] buffer = ArrayPool.Shared.Rent(DefaultCopyBufferSize); + try + { + while (true) + { + cancellationTokenSource.CancelAfter(timeout); +#pragma warning disable CA1835 // ReadAsync(Memory<>) overload is not available in all targets + int bytesRead = await source.ReadAsync(buffer, 0, buffer.Length, cancellationTokenSource.Token).ConfigureAwait(false); +#pragma warning restore // ReadAsync(Memory<>) overload is not available in all targets + if (bytesRead == 0) break; + await destination.WriteAsync(new ReadOnlyMemory(buffer, 0, bytesRead), cancellationTokenSource.Token).ConfigureAwait(false); + } + } + finally + { + cancellationTokenSource.CancelAfter(Timeout.InfiniteTimeSpan); + ArrayPool.Shared.Return(buffer); + } + } + + private static void CopyTo(Stream source, Stream destination, TimeSpan timeout, CancellationTokenSource cancellationTokenSource) + { + byte[] buffer = ArrayPool.Shared.Rent(DefaultCopyBufferSize); + try + { + int read; + while ((read = source.Read(buffer, 0, buffer.Length)) != 0) + { + cancellationTokenSource.Token.ThrowIfCancellationRequested(); + cancellationTokenSource.CancelAfter(timeout); + destination.Write(buffer, 0, read); + } + } + finally + { + cancellationTokenSource.CancelAfter(Timeout.InfiniteTimeSpan); + ArrayPool.Shared.Return(buffer); + } + } + + #endregion } From 507b9d95142bc2fcc6d9b6a7288c09b6edc4ee18 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Thu, 4 Jan 2024 17:49:38 -0800 Subject: [PATCH 12/17] Make result types non-abstract --- .../api/System.ClientModel.net6.0.cs | 10 +++---- .../api/System.ClientModel.netstandard2.0.cs | 10 +++---- .../src/Convenience/ClientResult.cs | 29 +++---------------- .../src/Convenience/ClientResultOfT.cs | 4 +-- .../Convenience/OptionalClientResultOfT.cs | 4 +-- 5 files changed, 18 insertions(+), 39 deletions(-) diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs index 7c1a4d07cb63..ea4202387648 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs @@ -1,6 +1,6 @@ namespace System.ClientModel { - public abstract partial class ClientResult + public partial class ClientResult { protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } public static System.ClientModel.OptionalClientResult FromOptionalValue(T? value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } @@ -17,9 +17,9 @@ public ClientResultException(string message, System.Exception? innerException = public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } } - public abstract partial class ClientResult : System.ClientModel.OptionalClientResult + public partial class ClientResult : System.ClientModel.OptionalClientResult { - protected ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } + protected internal ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] public sealed override bool HasValue { get { throw null; } } public sealed override T Value { get { throw null; } } @@ -30,9 +30,9 @@ public KeyCredential(string key) { } public string GetValue() { throw null; } public void Update(string key) { } } - public abstract partial class OptionalClientResult : System.ClientModel.ClientResult + public partial class OptionalClientResult : System.ClientModel.ClientResult { - protected OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } + protected internal OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } public virtual bool HasValue { get { throw null; } } public virtual T? Value { get { throw null; } } } diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs index a5aa6e4bac08..e347454acbf6 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs @@ -1,6 +1,6 @@ namespace System.ClientModel { - public abstract partial class ClientResult + public partial class ClientResult { protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } public static System.ClientModel.OptionalClientResult FromOptionalValue(T? value, System.ClientModel.Primitives.PipelineResponse response) { throw null; } @@ -17,9 +17,9 @@ public ClientResultException(string message, System.Exception? innerException = public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } } - public abstract partial class ClientResult : System.ClientModel.OptionalClientResult + public partial class ClientResult : System.ClientModel.OptionalClientResult { - protected ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } + protected internal ClientResult(T value, System.ClientModel.Primitives.PipelineResponse response) : base (default(T), default(System.ClientModel.Primitives.PipelineResponse)) { } [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] public sealed override bool HasValue { get { throw null; } } public sealed override T Value { get { throw null; } } @@ -30,9 +30,9 @@ public KeyCredential(string key) { } public string GetValue() { throw null; } public void Update(string key) { } } - public abstract partial class OptionalClientResult : System.ClientModel.ClientResult + public partial class OptionalClientResult : System.ClientModel.ClientResult { - protected OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } + protected internal OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } public virtual bool HasValue { get { throw null; } } public virtual T? Value { get { throw null; } } } diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs index 334556accb53..1fb0d5518212 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs @@ -6,7 +6,7 @@ namespace System.ClientModel; -public abstract class ClientResult +public class ClientResult { private readonly PipelineResponse _response; @@ -26,7 +26,7 @@ protected ClientResult(PipelineResponse response) #region Factory methods for ClientResult and subtypes public static ClientResult FromResponse(PipelineResponse response) - => new ClientModelClientResult(response); + => new ClientResult(response); public static ClientResult FromValue(T value, PipelineResponse response) { @@ -39,32 +39,11 @@ public static ClientResult FromValue(T value, PipelineResponse response) throw new ArgumentNullException(nameof(value), message); } - return new ClientModelClientResult(value, response); + return new ClientResult(value, response); } public static OptionalClientResult FromOptionalValue(T? value, PipelineResponse response) - => new ClientModelOptionalClientResult(value, response); - - #endregion - - #region Private implementation subtypes of abstract ClientResult types - private class ClientModelClientResult : ClientResult - { - public ClientModelClientResult(PipelineResponse response) - : base(response) { } - } - - private class ClientModelOptionalClientResult : OptionalClientResult - { - public ClientModelOptionalClientResult(T? value, PipelineResponse response) - : base(value, response) { } - } - - private class ClientModelClientResult : ClientResult - { - public ClientModelClientResult(T value, PipelineResponse response) - : base(value, response) { } - } + => new OptionalClientResult(value, response); #endregion } diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs index ccaf2dedd931..0d288d8015d9 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs @@ -7,9 +7,9 @@ namespace System.ClientModel; -public abstract class ClientResult : OptionalClientResult +public class ClientResult : OptionalClientResult { - protected ClientResult(T value, PipelineResponse response) + protected internal ClientResult(T value, PipelineResponse response) : base(value, response) { // Null values must use OptionalClientResult diff --git a/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs index 5cbe914c52e2..c8de1521c8da 100644 --- a/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs +++ b/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs @@ -5,11 +5,11 @@ namespace System.ClientModel; -public abstract class OptionalClientResult : ClientResult +public class OptionalClientResult : ClientResult { private readonly T? _value; - protected OptionalClientResult(T? value, PipelineResponse response) : base(response) + protected internal OptionalClientResult(T? value, PipelineResponse response) : base(response) => _value = value; /// From ffd0b289cfbd24ab6a276625dfd4705c46c2f63b Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Fri, 5 Jan 2024 08:59:41 -0800 Subject: [PATCH 13/17] rework exception constructors --- .../api/System.ClientModel.net6.0.cs | 4 ++-- .../api/System.ClientModel.netstandard2.0.cs | 4 ++-- .../src/Convenience/ClientResultException.cs | 21 +++++++------------ .../ClientRequestExceptionTests.cs | 2 +- 4 files changed, 12 insertions(+), 19 deletions(-) diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs index ea4202387648..229127335282 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs @@ -10,9 +10,9 @@ protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) } public partial class ClientResultException : System.Exception, System.Runtime.Serialization.ISerializable { - public ClientResultException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } + public ClientResultException(System.ClientModel.Primitives.PipelineResponse response, System.Exception? innerException = null) { } protected ClientResultException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } - public ClientResultException(string message, System.Exception? innerException = null) { } + public ClientResultException(string message, System.ClientModel.Primitives.PipelineResponse? response = null, System.Exception? innerException = null) { } public int Status { get { throw null; } protected set { } } public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs index e347454acbf6..66c02dfa9d33 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs @@ -10,9 +10,9 @@ protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) } public partial class ClientResultException : System.Exception, System.Runtime.Serialization.ISerializable { - public ClientResultException(System.ClientModel.Primitives.PipelineResponse response, string? message = null, System.Exception? innerException = null) { } + public ClientResultException(System.ClientModel.Primitives.PipelineResponse response, System.Exception? innerException = null) { } protected ClientResultException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } - public ClientResultException(string message, System.Exception? innerException = null) { } + public ClientResultException(string message, System.ClientModel.Primitives.PipelineResponse? response = null, System.Exception? innerException = null) { } public int Status { get { throw null; } protected set { } } public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } public System.ClientModel.Primitives.PipelineResponse? GetRawResponse() { throw null; } diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs index eeb52fabc62c..d04f726bdbde 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs @@ -4,7 +4,6 @@ using System.ClientModel.Internal; using System.ClientModel.Primitives; using System.Globalization; -using System.IO; using System.Runtime.Serialization; using System.Text; @@ -27,8 +26,8 @@ public int Status protected set => _status = value; } - public ClientResultException(PipelineResponse response, string? message = default, Exception? innerException = default) - : base(GetMessage(response, message), innerException) + public ClientResultException(PipelineResponse response, Exception? innerException = default) + : base(GetMessage(response), innerException) { ClientUtilities.AssertNotNull(response, nameof(response)); @@ -36,10 +35,11 @@ public ClientResultException(PipelineResponse response, string? message = defaul _status = response.Status; } - public ClientResultException(string message, Exception? innerException = default) - : base(message, innerException) + public ClientResultException(string message, PipelineResponse? response = default, Exception? innerException = default) + : base(message ?? DefaultMessage, innerException) { - _status = 0; + _response = response; + _status = response?.Status ?? 0; } /// @@ -65,15 +65,8 @@ public override void GetObjectData(SerializationInfo info, StreamingContext cont public PipelineResponse? GetRawResponse() => _response; - // Create message from response if available, and override message, if available. - private static string GetMessage(PipelineResponse response, string? message) + private static string GetMessage(PipelineResponse response) { - // Setting the message will override extracting it from the response. - if (message is not null) - { - return message; - } - response.BufferContent(); StringBuilder messageBuilder = new(); diff --git a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs index 176a296fe177..173090c729d9 100644 --- a/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs +++ b/sdk/core/System.ClientModel/tests/Convenience/ClientRequestExceptionTests.cs @@ -30,7 +30,7 @@ public void PassingMessageOverridesResponseMessage() PipelineResponse response = new MockPipelineResponse(200, "MockReason"); string message = "Override Message"; - ClientResultException exception = new ClientResultException(response, message); + ClientResultException exception = new ClientResultException(message, response); Assert.AreEqual(response.Status, exception.Status); Assert.AreEqual(response, exception.GetRawResponse()); From 55efe8322d4e09332b736148cbaed1834e749fac Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Fri, 5 Jan 2024 16:25:58 -0800 Subject: [PATCH 14/17] address pr feedback --- sdk/core/System.ClientModel/README.md | 2 +- .../System.ClientModel/src/Convenience/ClientResult.cs | 2 +- .../src/Convenience/ClientResultException.cs | 4 ++-- .../src/Convenience/ClientResultOfT.cs | 9 +++++++-- .../System.ClientModel/src/Convenience/KeyCredential.cs | 4 ++-- .../src/Convenience/OptionalClientResultOfT.cs | 2 +- 6 files changed, 14 insertions(+), 9 deletions(-) diff --git a/sdk/core/System.ClientModel/README.md b/sdk/core/System.ClientModel/README.md index 357746e2eb09..8fa8dfda86cc 100644 --- a/sdk/core/System.ClientModel/README.md +++ b/sdk/core/System.ClientModel/README.md @@ -25,7 +25,7 @@ None needed for `System.ClientModel`. ### Authenticate the client -The `System.ClientModel` preview package provides a `KeyCredential` type for authentication. +The `System.ClientModel` package provides a `KeyCredential` type for authentication. ## Key concepts diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs index 1fb0d5518212..47e129ae7c43 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs @@ -34,7 +34,7 @@ public static ClientResult FromValue(T value, PipelineResponse response) if (value is null) { string message = "ClientResult contract guarantees that ClientResult.Value is non-null. " + - "If you need to return an ClientResult where the Value is null, please use OptionalClientResult instead."; + "If you need to return a ClientResult where the Value is null, please use OptionalClientResult instead."; throw new ArgumentNullException(nameof(value), message); } diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs index d04f726bdbde..ff9afca7bd96 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs @@ -27,7 +27,7 @@ public int Status } public ClientResultException(PipelineResponse response, Exception? innerException = default) - : base(GetMessage(response), innerException) + : base(CreateMessage(response), innerException) { ClientUtilities.AssertNotNull(response, nameof(response)); @@ -65,7 +65,7 @@ public override void GetObjectData(SerializationInfo info, StreamingContext cont public PipelineResponse? GetRawResponse() => _response; - private static string GetMessage(PipelineResponse response) + private static string CreateMessage(PipelineResponse response) { response.BufferContent(); diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs index 0d288d8015d9..f5067dac3744 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultOfT.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -using System.ClientModel.Internal; using System.ClientModel.Primitives; using System.ComponentModel; @@ -13,7 +12,13 @@ protected internal ClientResult(T value, PipelineResponse response) : base(value, response) { // Null values must use OptionalClientResult - ClientUtilities.AssertNotNull(value, nameof(value)); + if (value is null) + { + string message = "ClientResult contract guarantees that ClientResult.Value is non-null. " + + "If you need to return a ClientResult where the Value is null, please use OptionalClientResult instead."; + + throw new ArgumentNullException(nameof(value), message); + } } public sealed override T Value => base.Value!; diff --git a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs b/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs index ec69ccbb7253..74a24196a470 100644 --- a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs +++ b/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System.ClientModel.Internal; using System.Threading; namespace System.ClientModel; @@ -30,8 +31,7 @@ public class KeyCredential public void Update(string key) { - if (key is null) throw new ArgumentNullException(nameof(key)); - if (key.Length == 0) throw new ArgumentException("Value cannot be an empty string.", nameof(key)); + ClientUtilities.AssertNotNullOrEmpty(key, nameof(key)); Volatile.Write(ref _key, key); } diff --git a/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs b/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs index c8de1521c8da..d086c70a13a9 100644 --- a/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs +++ b/sdk/core/System.ClientModel/src/Convenience/OptionalClientResultOfT.cs @@ -18,7 +18,7 @@ protected internal OptionalClientResult(T? value, PipelineResponse response) : b public virtual T? Value => _value; /// - /// Gets a value indicating whether the current instance has a valid value of its underlying type. + /// Gets a value indicating whether the current instance has a non-null value. /// public virtual bool HasValue => _value != null; } From 14dcdba0ad6a348ce6dc5d19b98c6fa3c251633e Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Fri, 5 Jan 2024 16:33:59 -0800 Subject: [PATCH 15/17] address pr feedback --- .../src/Convenience/ClientResult.cs | 2 +- .../src/Convenience/ClientResultException.cs | 4 +- .../src/Convenience/KeyCredential.cs | 2 +- .../src/Internal/ClientUtilities.cs | 91 ------------------- .../src/Internal/StreamExtensions.cs | 4 +- 5 files changed, 6 insertions(+), 97 deletions(-) delete mode 100644 sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs index 47e129ae7c43..61f4d7ab4a96 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResult.cs @@ -12,7 +12,7 @@ public class ClientResult protected ClientResult(PipelineResponse response) { - ClientUtilities.AssertNotNull(response, nameof(response)); + Argument.AssertNotNull(response, nameof(response)); _response = response; } diff --git a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs index ff9afca7bd96..bf86832f1dba 100644 --- a/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ClientResultException.cs @@ -29,7 +29,7 @@ public int Status public ClientResultException(PipelineResponse response, Exception? innerException = default) : base(CreateMessage(response), innerException) { - ClientUtilities.AssertNotNull(response, nameof(response)); + Argument.AssertNotNull(response, nameof(response)); _response = response; _status = response.Status; @@ -56,7 +56,7 @@ protected ClientResultException(SerializationInfo info, StreamingContext context /// public override void GetObjectData(SerializationInfo info, StreamingContext context) { - ClientUtilities.AssertNotNull(info, nameof(info)); + Argument.AssertNotNull(info, nameof(info)); info.AddValue(nameof(Status), Status); diff --git a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs b/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs index 74a24196a470..28552545be06 100644 --- a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs +++ b/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs @@ -31,7 +31,7 @@ public class KeyCredential public void Update(string key) { - ClientUtilities.AssertNotNullOrEmpty(key, nameof(key)); + Argument.AssertNotNullOrEmpty(key, nameof(key)); Volatile.Write(ref _key, key); } diff --git a/sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs b/sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs deleted file mode 100644 index 18453754a99b..000000000000 --- a/sdk/core/System.ClientModel/src/Internal/ClientUtilities.cs +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -using System.Threading; -using System.Threading.Tasks; - -namespace System.ClientModel.Internal; - -internal class ClientUtilities -{ - #region Argument validation - public static void AssertNotNull(T value, string name) - { - if (value is null) - { - throw new ArgumentNullException(name); - } - } - - public static void AssertNotNullOrEmpty(string value, string name) - { - if (value is null) - { - throw new ArgumentNullException(name); - } - - if (value.Length == 0) - { - throw new ArgumentException("Value cannot be an empty string.", name); - } - } - - /// - /// Throws if is less than the or greater than the . - /// - /// The type of to validate which implements . - /// The value to validate. - /// The minimum value to compare. - /// The maximum value to compare. - /// The name of the parameter. - public static void AssertInRange(T value, T minimum, T maximum, string name) where T : notnull, IComparable - { - if (minimum.CompareTo(value) > 0) - { - throw new ArgumentOutOfRangeException(name, "Value is less than the minimum allowed."); - } - - if (maximum.CompareTo(value) < 0) - { - throw new ArgumentOutOfRangeException(name, "Value is greater than the maximum allowed."); - } - } - #endregion - - #region CancellationToken helpers - - /// The default message used by . - private static readonly string s_cancellationMessage = new OperationCanceledException().Message; // use same message as the default ctor - - /// Determines whether to wrap an in a cancellation exception. - /// The exception. - /// The that may have triggered the exception. - /// true if the exception should be wrapped; otherwise, false. - public static bool ShouldWrapInOperationCanceledException(Exception exception, CancellationToken cancellationToken) => - !(exception is OperationCanceledException) && cancellationToken.IsCancellationRequested; - - /// Throws a cancellation exception if cancellation has been requested via . - /// The token to check for a cancellation request. - public static void ThrowIfCancellationRequested(CancellationToken cancellationToken) - { - if (cancellationToken.IsCancellationRequested) - { - ThrowOperationCanceledException(innerException: null, cancellationToken); - } - } - - /// Throws a cancellation exception. - /// The inner exception to wrap. May be null. - /// The that triggered the cancellation. - private static void ThrowOperationCanceledException(Exception? innerException, CancellationToken cancellationToken) => - throw CreateOperationCanceledException(innerException, cancellationToken); - - public static Exception CreateOperationCanceledException(Exception? innerException, CancellationToken cancellationToken, string? message = null) => -#if NET6_0_OR_GREATER - new TaskCanceledException(message ?? s_cancellationMessage, innerException, cancellationToken); // TCE for compatibility with other handlers that use TaskCompletionSource.TrySetCanceled() -#else - new TaskCanceledException(message ?? s_cancellationMessage, innerException); -#endif - - #endregion -} diff --git a/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs b/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs index b0b70270fbd0..21fdef014f56 100644 --- a/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs +++ b/sdk/core/System.ClientModel/src/Internal/StreamExtensions.cs @@ -14,7 +14,7 @@ internal static class StreamExtensions { public static async Task WriteAsync(this Stream stream, ReadOnlyMemory buffer, CancellationToken cancellation = default) { - ClientUtilities.AssertNotNull(stream, nameof(stream)); + Argument.AssertNotNull(stream, nameof(stream)); #if NETCOREAPP await stream.WriteAsync(buffer, cancellation).ConfigureAwait(false); #else @@ -48,7 +48,7 @@ public static async Task WriteAsync(this Stream stream, ReadOnlyMemory buf public static async Task WriteAsync(this Stream stream, ReadOnlySequence buffer, CancellationToken cancellation = default) { - ClientUtilities.AssertNotNull(stream, nameof(stream)); + Argument.AssertNotNull(stream, nameof(stream)); if (buffer.Length == 0) return; From 3b8ce8c82342352f95013f94951ec16bbfa622b9 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Fri, 5 Jan 2024 16:59:09 -0800 Subject: [PATCH 16/17] rename KeyCredential and change GetValue to Deconstruct --- .../api/System.ClientModel.net6.0.cs | 12 ++--- .../api/System.ClientModel.netstandard2.0.cs | 12 ++--- .../{KeyCredential.cs => ApiKeyCredential.cs} | 17 ++++--- .../src/Internal/Argument.cs | 51 +++++++++++++++++++ 4 files changed, 73 insertions(+), 19 deletions(-) rename sdk/core/System.ClientModel/src/Convenience/{KeyCredential.cs => ApiKeyCredential.cs} (57%) create mode 100644 sdk/core/System.ClientModel/src/Internal/Argument.cs diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs index 229127335282..a912e025bc53 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs @@ -1,5 +1,11 @@ namespace System.ClientModel { + public partial class ApiKeyCredential + { + public ApiKeyCredential(string key) { } + public void Deconstruct(out string key) { throw null; } + public void Update(string key) { } + } public partial class ClientResult { protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } @@ -24,12 +30,6 @@ public partial class ClientResult : System.ClientModel.OptionalClientResult : System.ClientModel.ClientResult { protected internal OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } diff --git a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs index 66c02dfa9d33..2e7bfe4c69bf 100644 --- a/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs +++ b/sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs @@ -1,5 +1,11 @@ namespace System.ClientModel { + public partial class ApiKeyCredential + { + public ApiKeyCredential(string key) { } + public void Deconstruct(out string key) { throw null; } + public void Update(string key) { } + } public partial class ClientResult { protected ClientResult(System.ClientModel.Primitives.PipelineResponse response) { } @@ -24,12 +30,6 @@ public partial class ClientResult : System.ClientModel.OptionalClientResult : System.ClientModel.ClientResult { protected internal OptionalClientResult(T? value, System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { } diff --git a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs b/sdk/core/System.ClientModel/src/Convenience/ApiKeyCredential.cs similarity index 57% rename from sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs rename to sdk/core/System.ClientModel/src/Convenience/ApiKeyCredential.cs index 28552545be06..38dbd4ec4695 100644 --- a/sdk/core/System.ClientModel/src/Convenience/KeyCredential.cs +++ b/sdk/core/System.ClientModel/src/Convenience/ApiKeyCredential.cs @@ -6,12 +6,12 @@ namespace System.ClientModel; -public class KeyCredential +public class ApiKeyCredential { private string _key; /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the class. /// /// Key to use to authenticate with the Azure service. /// @@ -21,13 +21,16 @@ public class KeyCredential /// Thrown when the is empty. /// #pragma warning disable CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable. - public KeyCredential(string key) => Update(key); + public ApiKeyCredential(string key) => Update(key); #pragma warning restore CS8618 // Non-nullable field is uninitialized. Consider declaring as nullable. - // 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); + // Note: this is uses a method instead of a Key property so if an instance of + // the type were passed to a serializer that serializes public properties + // it doesn't inadvertently expose a secret. We call it Deconstruct to allow + // consistent naming across credential types. For example, for + // NamedKeyCredential, we expect to have a Deconstruct method with two out + // params to enable atomicity in reading multiple credential values. + public void Deconstruct(out string key) => key = Volatile.Read(ref _key); public void Update(string key) { diff --git a/sdk/core/System.ClientModel/src/Internal/Argument.cs b/sdk/core/System.ClientModel/src/Internal/Argument.cs new file mode 100644 index 000000000000..f88f5a5420ba --- /dev/null +++ b/sdk/core/System.ClientModel/src/Internal/Argument.cs @@ -0,0 +1,51 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +namespace System.ClientModel.Internal; + +internal class Argument +{ + #region Argument validation + public static void AssertNotNull(T value, string name) + { + if (value is null) + { + throw new ArgumentNullException(name); + } + } + + public static void AssertNotNullOrEmpty(string value, string name) + { + if (value is null) + { + throw new ArgumentNullException(name); + } + + if (value.Length == 0) + { + throw new ArgumentException("Value cannot be an empty string.", name); + } + } + + /// + /// Throws if is less than the or greater than the . + /// + /// The type of to validate which implements . + /// The value to validate. + /// The minimum value to compare. + /// The maximum value to compare. + /// The name of the parameter. + public static void AssertInRange(T value, T minimum, T maximum, string name) where T : notnull, IComparable + { + if (minimum.CompareTo(value) > 0) + { + throw new ArgumentOutOfRangeException(name, "Value is less than the minimum allowed."); + } + + if (maximum.CompareTo(value) < 0) + { + throw new ArgumentOutOfRangeException(name, "Value is greater than the maximum allowed."); + } + } + #endregion +} From d464a87cd64ee7c1ac9054a2c6d783ada54758f4 Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Mon, 8 Jan 2024 10:10:25 -0800 Subject: [PATCH 17/17] Add tests for Argument.cs --- .../tests/internal/Internal/ArgumentTests.cs | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 sdk/core/System.ClientModel/tests/internal/Internal/ArgumentTests.cs diff --git a/sdk/core/System.ClientModel/tests/internal/Internal/ArgumentTests.cs b/sdk/core/System.ClientModel/tests/internal/Internal/ArgumentTests.cs new file mode 100644 index 000000000000..2d2aac53e86e --- /dev/null +++ b/sdk/core/System.ClientModel/tests/internal/Internal/ArgumentTests.cs @@ -0,0 +1,90 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using NUnit.Framework; +using System; +using System.ClientModel.Internal; + +namespace Azure.Core.Tests +{ + public class ArgumentTests + { + [TestCase("test")] + public void NotNull(object? value) + { + Argument.AssertNotNull(value, "value"); + + Assert.AreEqual("test", value!.ToString()); + } + + [Test] + public void NotNullThrowsOnNull() + { + object? value = null; + Assert.Throws(() => Argument.AssertNotNull(value, "value")); + } + + [Test] + public void NotNullNullableInt32() + { + int? value = 1; + Argument.AssertNotNull(value, "value"); + } + + [Test] + public void NotNullNullableInt32ThrowsOnNull() + { + int? value = null; + Assert.Throws(() => Argument.AssertNotNull(value, "value")); + } + + [Test] + public void NotNullOrEmptyString() + { + string value = "test"; + Argument.AssertNotNullOrEmpty(value, "value"); + } + + [Test] + public void NotNullOrEmptyStringThrowsOnNull() + { + string? value = null; + Assert.Throws(() => Argument.AssertNotNullOrEmpty(value!, "value")); + } + + [Test] + public void NotNullOrEmptyStringThrowsOnEmpty() + { + Assert.Throws(() => Argument.AssertNotNullOrEmpty(string.Empty, "value")); + } + + [TestCase(0, 0, 2)] + [TestCase(1, 0, 2)] + [TestCase(2, 0, 2)] + public void InRangeInt32(int value, int minimum, int maximum) + { + Argument.AssertInRange(value, minimum, maximum, "value"); + } + + [TestCase(-1, 0, 2)] + [TestCase(3, 0, 2)] + public void InRangeInt32Throws(int value, int minimum, int maximum) + { + Assert.Throws(() => Argument.AssertInRange(value, minimum, maximum, "value")); + } + + private readonly struct TestStructure : IEquatable + { + internal readonly string A; + internal readonly int B; + + internal TestStructure(string a, int b) + { + A = a; + B = b; + } + + public bool Equals(TestStructure other) => string.Equals(A, other.A, StringComparison.Ordinal) && B == other.B; + } + } +}