-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: users/annelo/synapse-rbac-llc
Are you sure you want to change the base?
Changes from 2 commits
153dd70
02c0fb2
ff6adbe
37c3bcf
6461b5d
5ec8cd2
43a9fee
3301093
c899c34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,10 @@ public HttpClientTransport(HttpClient client) | |
/// </summary> | ||
public static readonly HttpClientTransport Shared = new HttpClientTransport(); | ||
|
||
internal ClientOptions? ClientOptions { get; set; } | ||
|
||
internal ResponseClassifier? ResponseClassifier { get; set; } | ||
|
||
/// <inheritdoc /> | ||
public sealed override Request CreateRequest() | ||
=> new PipelineRequest(); | ||
|
@@ -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() | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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); | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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), | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't use a |
||
//break; | ||
} | ||
} | ||
|
||
private static SynapseRoleAssignment DeserializeResponse(Response response) | ||
{ | ||
throw new NotImplementedException(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Implement this |
||
} | ||
} | ||
} |
There was a problem hiding this comment.
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