Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLC investigation into casting Response type to models #3

Draft
wants to merge 9 commits into
base: users/annelo/synapse-rbac-llc
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions sdk/core/Azure.Core/src/Pipeline/HttpClientTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ public HttpClientTransport(HttpClient client)
/// </summary>
public static readonly HttpClientTransport Shared = new HttpClientTransport();

internal ClientOptions? ClientOptions { get; set; }
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't have to be ClientOptions, it could just be HttpMessageSanitizer


internal ResponseClassifier? ResponseClassifier { get; set; }

/// <inheritdoc />
public sealed override Request CreateRequest()
=> new PipelineRequest();
Expand Down Expand Up @@ -129,7 +133,7 @@ private async ValueTask ProcessAsync(HttpMessage message, bool async)
throw new RequestFailedException(e.Message, e);
}

message.Response = new PipelineResponse(message.Request.ClientRequestId, responseMessage, contentStream);
message.Response = new PipelineResponse(message.Request.ClientRequestId, responseMessage, contentStream, this.ClientOptions!, this.ResponseClassifier!);
}

private static HttpClient CreateDefaultClient()
Expand Down Expand Up @@ -500,9 +504,11 @@ private sealed class PipelineResponse : Response
private Stream? _contentStream;
#pragma warning restore CA2213

public PipelineResponse(string requestId, HttpResponseMessage responseMessage, Stream? contentStream)
public PipelineResponse(string requestId, HttpResponseMessage responseMessage, Stream? contentStream, ClientOptions options, ResponseClassifier classifier)
{
ClientRequestId = requestId ?? throw new ArgumentNullException(nameof(requestId));
ExceptionFactory = new ResponseExceptionFactory(options);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't want an allocation here? I think I can refactor to achieve that.

ResponseClassifier = classifier;
_responseMessage = responseMessage ?? throw new ArgumentNullException(nameof(responseMessage));
_contentStream = contentStream;
_responseContent = _responseMessage.Content;
Expand All @@ -526,6 +532,10 @@ public override Stream? ContentStream

public override string ClientRequestId { get; set; }

internal override ResponseExceptionFactory ExceptionFactory { get; }

internal override ResponseClassifier ResponseClassifier { get; }

protected internal override bool TryGetHeader(string name, [NotNullWhen(true)] out string? value) => HttpClientTransport.TryGetHeader(_responseMessage.Headers, _responseContent, name, out value);

protected internal override bool TryGetHeaderValues(string name, [NotNullWhen(true)] out IEnumerable<string>? values) => HttpClientTransport.TryGetHeader(_responseMessage.Headers, _responseContent, name, out values);
Expand Down
1 change: 1 addition & 0 deletions sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public HttpPipeline(HttpPipelineTransport transport, HttpPipelinePolicy[]? polic
policies = policies ?? Array.Empty<HttpPipelinePolicy>();

var all = new HttpPipelinePolicy[policies.Length + 1];
_transport.ResponseClassifier = ResponseClassifier;
all[policies.Length] = new HttpPipelineTransportPolicy(_transport);
policies.CopyTo(all, 0);

Expand Down
6 changes: 6 additions & 0 deletions sdk/core/Azure.Core/src/Pipeline/HttpPipelineTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,11 @@ public abstract class HttpPipelineTransport
/// </summary>
/// <returns></returns>
public abstract Request CreateRequest();


internal ClientOptions? ClientOptions { get; set; }

internal ResponseClassifier? ResponseClassifier { get; set; }

}
}
24 changes: 24 additions & 0 deletions sdk/core/Azure.Core/src/Response.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Threading.Tasks;
using Azure.Core;
using Azure.Core.Pipeline;

namespace Azure
{
Expand Down Expand Up @@ -111,6 +113,28 @@ public virtual BinaryData Content
/// <returns>The <see cref="IEnumerable{T}"/> enumerating <see cref="HttpHeader"/> in the response.</returns>
protected internal abstract IEnumerable<HttpHeader> EnumerateHeaders();

internal abstract ResponseExceptionFactory ExceptionFactory { get; }

internal abstract ResponseClassifier ResponseClassifier { get; }

/// <summary>
/// Throw a RequestFailedException appropriate to the Response.
/// </summary>
public void Throw()
{
throw this.ExceptionFactory.CreateRequestFailedException(this);
//throw new RequestFailedException("<error message>");
}

/// <summary>
/// Throw a RequestFailedException appropriate to the Response.
/// </summary>
public async Task ThrowAsync()
{
throw await this.ExceptionFactory.CreateRequestFailedExceptionAsync(this).ConfigureAwait(false);
//throw new RequestFailedException("<error message>");
}

/// <summary>
/// Creates a new instance of <see cref="Response{T}"/> with the provided value and HTTP response.
/// </summary>
Expand Down
184 changes: 184 additions & 0 deletions sdk/core/Azure.Core/src/ResponseExceptionFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text;
using System.Text.Json;
using System.Threading.Tasks;

namespace Azure.Core.Pipeline
{
internal class ResponseExceptionFactory
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially what's in ClientDiagnostics, just one approach to factoring out the logic (that I'm not committed to if we don't like it)

{
private const string DefaultMessage = "Service request failed.";

private readonly HttpMessageSanitizer _sanitizer;

public ResponseExceptionFactory(ClientOptions options)
{
_sanitizer = new HttpMessageSanitizer(
options.Diagnostics.LoggedQueryParameters.ToArray(),
options.Diagnostics.LoggedHeaderNames.ToArray());
}

public async ValueTask<RequestFailedException> CreateRequestFailedExceptionAsync(Response response, string? message = null, string? errorCode = null, IDictionary<string, string>? additionalInfo = null, Exception? innerException = null)
{
var content = await ReadContentAsync(response, true).ConfigureAwait(false);
ExtractFailureContent(content, ref message, ref errorCode);
return CreateRequestFailedExceptionWithContent(response, message, content, errorCode, additionalInfo, innerException);
}

public RequestFailedException CreateRequestFailedException(Response response, string? message = null, string? errorCode = null, IDictionary<string, string>? additionalInfo = null, Exception? innerException = null)
{
string? content = ReadContentAsync(response, false).EnsureCompleted();
ExtractFailureContent(content, ref message, ref errorCode);
return CreateRequestFailedExceptionWithContent(response, message, content, errorCode, additionalInfo, innerException);
}

private static async ValueTask<string?> ReadContentAsync(Response response, bool async)
{
string? content = null;

if (response.ContentStream != null &&
ContentTypeUtilities.TryGetTextEncoding(response.Headers.ContentType, out var encoding))
{
using (var streamReader = new StreamReader(response.ContentStream, encoding))
{
content = async ? await streamReader.ReadToEndAsync().ConfigureAwait(false) : streamReader.ReadToEnd();
}
}

return content;
}

private static void ExtractFailureContent(
string? content,
ref string? message,
ref string? errorCode)
{
try
{
// Optimistic check for JSON object we expect
if (content == null ||
!content.StartsWith("{", StringComparison.OrdinalIgnoreCase))
return;

string? parsedMessage = null;
using JsonDocument document = JsonDocument.Parse(content);
if (document.RootElement.TryGetProperty("error", out var errorProperty))
{
if (errorProperty.TryGetProperty("code", out var codeProperty))
{
errorCode = codeProperty.GetString();
}
if (errorProperty.TryGetProperty("message", out var messageProperty))
{
parsedMessage = messageProperty.GetString();
}
}

// Make sure we parsed a message so we don't overwrite the value with null
if (parsedMessage != null)
{
message = parsedMessage;
}
}
catch (Exception)
{
// Ignore any failures - unexpected content will be
// included verbatim in the detailed error message
}
}

private RequestFailedException CreateRequestFailedExceptionWithContent(
Response response,
string? message = null,
string? content = null,
string? errorCode = null,
IDictionary<string, string>? additionalInfo = null,
Exception? innerException = null)
{
var formatMessage = CreateRequestFailedMessageWithContent(response, message, content, errorCode, additionalInfo);
var exception = new RequestFailedException(response.Status, formatMessage, errorCode, innerException);

if (additionalInfo != null)
{
foreach (KeyValuePair<string, string> keyValuePair in additionalInfo)
{
exception.Data.Add(keyValuePair.Key, keyValuePair.Value);
}
}

return exception;
}

private string CreateRequestFailedMessageWithContent(
Response response,
string? message,
string? content,
string? errorCode,
IDictionary<string, string>? additionalInfo)
{
StringBuilder messageBuilder = new StringBuilder()
.AppendLine(message ?? DefaultMessage)
.Append("Status: ")
.Append(response.Status.ToString(CultureInfo.InvariantCulture));

if (!string.IsNullOrEmpty(response.ReasonPhrase))
{
messageBuilder.Append(" (")
.Append(response.ReasonPhrase)
.AppendLine(")");
}
else
{
messageBuilder.AppendLine();
}

if (!string.IsNullOrWhiteSpace(errorCode))
{
messageBuilder.Append("ErrorCode: ")
.Append(errorCode)
.AppendLine();
}

if (additionalInfo != null && additionalInfo.Count > 0)
{
messageBuilder
.AppendLine()
.AppendLine("Additional Information:");
foreach (KeyValuePair<string, string> info in additionalInfo)
{
messageBuilder
.Append(info.Key)
.Append(": ")
.AppendLine(info.Value);
}
}

if (content != null)
{
messageBuilder
.AppendLine()
.AppendLine("Content:")
.AppendLine(content);
}

messageBuilder
.AppendLine()
.AppendLine("Headers:");

foreach (HttpHeader responseHeader in response.Headers)
{
string headerValue = _sanitizer.SanitizeHeader(responseHeader.Name, responseHeader.Value);
messageBuilder.AppendLine($"{responseHeader.Name}: {headerValue}");
}

return messageBuilder.ToString();
}
}
}
6 changes: 2 additions & 4 deletions sdk/core/Azure.Core/src/Shared/ClientDiagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,13 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Net.Http.Headers;
using System.Reflection;
using System.Text;
using System.Text.Json;
using System.Threading.Tasks;
using Azure.Core.Pipeline;

#nullable enable

Expand All @@ -23,6 +20,7 @@ internal class ClientDiagnostics : DiagnosticScopeFactory
private const string DefaultMessage = "Service request failed.";

private readonly HttpMessageSanitizer _sanitizer;

public ClientDiagnostics(ClientOptions options) : base(
options.GetType().Namespace!,
GetResourceProviderNamespace(options.GetType().Assembly),
Expand Down Expand Up @@ -124,7 +122,7 @@ public async ValueTask<string> CreateRequestFailedMessageAsync(Response response
return CreateRequestFailedMessageWithContent(response, message, content, errorCode, additionalInfo);
}

public string CreateRequestFailedMessageWithContent(Response response, string? message, string? content, string? errorCode, IDictionary<string, string>? additionalInfo)
private string CreateRequestFailedMessageWithContent(Response response, string? message, string? content, string? errorCode, IDictionary<string, string>? additionalInfo)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found nothing that depends on this.

{
StringBuilder messageBuilder = new StringBuilder()
.AppendLine(message ?? DefaultMessage)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
<Import Project="..\..\Azure.Analytics.Synapse.Shared\src\Azure.Analytics.Synapse.Shared.projitems" Label="Shared" />

<ItemGroup>
<PackageReference Include="Azure.Core" />
<PackageReference Include="Azure.Core.Experimental" />
<PackageReference Include="System.Text.Json" />
</ItemGroup>

Expand All @@ -38,5 +36,9 @@
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\core\Azure.Core.Experimental\src\Azure.Core.Experimental.csproj" />
<ProjectReference Include="..\..\..\core\Azure.Core\src\Azure.Core.csproj" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,27 @@ public static implicit operator RequestContent(SynapseRoleAssignment value) => R
Id = value.Id,
Properties = value.Properties
});

public static implicit operator SynapseRoleAssignment(Response response)
{
switch (response.Status)
{
case 200:
return DeserializeResponse(response);
default:

#pragma warning disable CA1065 // Do not raise exceptions in unexpected locations
throw new NotImplementedException();
#pragma warning restore CA1065 // Do not raise exceptions in unexpected locations

//response.Throw();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't use a throw statement here, we get an error that not all paths return a value

//break;
}
}

private static SynapseRoleAssignment DeserializeResponse(Response response)
{
throw new NotImplementedException();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Implement this

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,9 @@
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\core\Azure.Core.Experimental\src\Azure.Core.Experimental.csproj" />
<ProjectReference Include="..\..\..\core\Azure.Core\src\Azure.Core.csproj" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,9 @@
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\core\Azure.Core.Experimental\src\Azure.Core.Experimental.csproj" />
<ProjectReference Include="..\..\..\core\Azure.Core\src\Azure.Core.csproj" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,9 @@
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureCoreSharedSources)AzureResourceProviderNamespaceAttribute.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\core\Azure.Core.Experimental\src\Azure.Core.Experimental.csproj" />
<ProjectReference Include="..\..\..\core\Azure.Core\src\Azure.Core.csproj" />
</ItemGroup>

</Project>
Loading