Skip to content

Commit 2a91880

Browse files
annelo-msftannelo-msft
authored
[LLC] Create RequestFailedException from Response (Azure#24283)
* use the Response.IsError property in an E2E test * add ResponsePropertiesPolicy into pipeline in generated code. * Trial implementation for discussion * update generated APIs and some nits * move exception creation to Experimental * update generated api listing * api updates * refactoring ClientDiagnostics per PR fb * pr fb * pr fb * pr fb
1 parent 63d1025 commit 2a91880

File tree

12 files changed

+196
-28
lines changed

12 files changed

+196
-28
lines changed

‎sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,7 @@ namespace Azure.Core.Pipeline
142142
{
143143
public static partial class ResponseExtensions
144144
{
145+
public static Azure.RequestFailedException CreateRequestFailedException(this Azure.Response response) { throw null; }
145146
public static bool IsError(this Azure.Response response) { throw null; }
146147
}
147148
}

‎sdk/core/Azure.Core.Experimental/src/Azure.Core.Experimental.csproj‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<Project Sdk="Microsoft.NET.Sdk">
1+
<Project Sdk="Microsoft.NET.Sdk">
22
<PropertyGroup>
33
<Description>Experimental types that might eventually move to Azure.Core</Description>
44
<AssemblyTitle>Microsoft Azure Client Pipeline Experimental Extensions</AssemblyTitle>
@@ -16,6 +16,12 @@
1616

1717
<ItemGroup>
1818
<Compile Include="$(AzureCoreSharedSources)Argument.cs" />
19+
<Compile Include="$(AzureCoreSharedSources)AppContextSwitchHelper.cs" />
20+
<Compile Include="$(AzureCoreSharedSources)ClientDiagnostics.cs" />
21+
<Compile Include="$(AzureCoreSharedSources)ContentTypeUtilities.cs" />
22+
<Compile Include="$(AzureCoreSharedSources)DiagnosticScope.cs" />
23+
<Compile Include="$(AzureCoreSharedSources)DiagnosticScopeFactory.cs" />
24+
<Compile Include="$(AzureCoreSharedSources)HttpMessageSanitizer.cs" />
1925
<Compile Include="$(AzureCoreSharedSources)HashCodeBuilder.cs" />
2026
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" />
2127
<Compile Include="$(AzureCoreSharedSources)NullableAttributes.cs" />

‎sdk/core/Azure.Core.Experimental/src/ClassifiedResponse.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
using System.Diagnostics.CodeAnalysis;
88
using System.IO;
99
using System.Text;
10+
using Azure.Core.Pipeline;
1011

1112
namespace Azure.Core
1213
{
@@ -23,6 +24,8 @@ public class ClassifiedResponse : Response
2324
/// </summary>
2425
public bool IsError { get; private set; }
2526

27+
internal ExceptionFormattingResponseClassifier? ResponseClassifier { get; set; }
28+
2629
internal void EvaluateError(HttpMessage message)
2730
{
2831
IsError = message.ResponseClassifier.IsErrorResponse(message);

‎sdk/core/Azure.Core.Experimental/src/ExceptionFormattingResponseClassifier.cs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Text;
7+
8+
namespace Azure.Core.Pipeline
9+
{
10+
/// <summary>
11+
/// Wrap ResponseClassifier and add exception formatting methods.
12+
/// </summary>
13+
internal class ExceptionFormattingResponseClassifier : ResponseClassifier
14+
{
15+
private ResponseClassifier _responseClassifier;
16+
17+
internal HttpMessageSanitizer MessageSanitizer { get; set; }
18+
19+
public override bool IsErrorResponse(HttpMessage message) => _responseClassifier.IsErrorResponse(message);
20+
21+
public override bool IsRetriable(HttpMessage message, Exception exception) => _responseClassifier.IsRetriable(message, exception);
22+
23+
public override bool IsRetriableException(Exception exception) => _responseClassifier.IsRetriableException(exception);
24+
25+
public override bool IsRetriableResponse(HttpMessage message) => _responseClassifier.IsRetriableResponse(message);
26+
27+
/// <summary>
28+
/// </summary>
29+
public ExceptionFormattingResponseClassifier(ResponseClassifier responseClassifier, DiagnosticsOptions diagnostics)
30+
{
31+
_responseClassifier = responseClassifier;
32+
MessageSanitizer = ClientDiagnostics.CreateMessageSanitizer(diagnostics);
33+
}
34+
}
35+
}

‎sdk/core/Azure.Core.Experimental/src/ResponseExtensions.cs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,5 +29,39 @@ public static bool IsError(this Response response)
2929

3030
return classifiedResponse.IsError;
3131
}
32+
33+
/// <summary>
34+
/// </summary>
35+
/// <param name="response"></param>
36+
/// <returns></returns>
37+
public static RequestFailedException CreateRequestFailedException(this Response response)
38+
{
39+
var classifiedResponse = response as ClassifiedResponse;
40+
41+
if (classifiedResponse == null)
42+
{
43+
throw new InvalidOperationException("ResponseClassifier was not set on the response. " +
44+
"Please ensure the pipeline includes ResponsePropertiesPolicy.");
45+
}
46+
47+
string message = null;
48+
string errorCode = null;
49+
50+
string content = ClientDiagnostics.ReadContentAsync(response, false).EnsureCompleted();
51+
ClientDiagnostics.ExtractAzureErrorContent(content, ref message, ref errorCode);
52+
string exceptionMessage = ClientDiagnostics.CreateRequestFailedMessageWithContent(
53+
response,
54+
message,
55+
content,
56+
errorCode,
57+
null,
58+
classifiedResponse.ResponseClassifier.MessageSanitizer);
59+
60+
return new RequestFailedException(
61+
response.Status,
62+
exceptionMessage,
63+
errorCode,
64+
null);
65+
}
3266
}
3367
}

‎sdk/core/Azure.Core.Experimental/src/ResponsePropertiesPolicy.cs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ namespace Azure.Core
1111
/// </summary>
1212
internal class ResponsePropertiesPolicy : HttpPipelinePolicy
1313
{
14+
private ClientOptions _clientOptions;
15+
16+
public ResponsePropertiesPolicy(ClientOptions options)
17+
{
18+
_clientOptions = options;
19+
}
20+
1421
/// <inheritdoc/>
1522
public override void Process(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline)
1623
{
@@ -23,7 +30,7 @@ public override ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpP
2330
return ProcessAsync(message, pipeline, true);
2431
}
2532

26-
private static async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline, bool async)
33+
private async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<HttpPipelinePolicy> pipeline, bool async)
2734
{
2835
if (async)
2936
{
@@ -39,6 +46,10 @@ private static async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory<
3946
ClassifiedResponse response = new ClassifiedResponse(message.Response);
4047
response.EvaluateError(message);
4148
message.Response = response;
49+
50+
// The non-experimental version of this functionality is roughly described in:
51+
// https://github.com/Azure/azure-sdk-for-net/pull/24248
52+
response.ResponseClassifier = new ExceptionFormattingResponseClassifier(message.ResponseClassifier, _clientOptions.Diagnostics);
4253
}
4354
}
4455
}

‎sdk/core/Azure.Core.Experimental/tests/Azure.Core.Experimental.Tests.csproj‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,6 @@
2323
<ItemGroup>
2424
<Compile Include="$(AzureCoreSharedSources)AppContextSwitchHelper.cs" LinkBase="Shared" />
2525
<Compile Include="$(AzureCoreSharedSources)ArrayBufferWriter.cs" LinkBase="Shared" />
26-
<Compile Include="$(AzureCoreSharedSources)ClientDiagnostics.cs" LinkBase="Shared" />
27-
<Compile Include="$(AzureCoreSharedSources)ContentTypeUtilities.cs" LinkBase="Shared" />
28-
<Compile Include="$(AzureCoreSharedSources)DiagnosticScope.cs" LinkBase="Shared" />
29-
<Compile Include="$(AzureCoreSharedSources)DiagnosticScopeFactory.cs" LinkBase="Shared" />
30-
<Compile Include="$(AzureCoreSharedSources)HttpMessageSanitizer.cs" LinkBase="Shared" />
3126
</ItemGroup>
3227

3328
</Project>

‎sdk/core/Azure.Core.Experimental/tests/LowLevelClient/LowLevelClientModels/Pet.cs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,15 @@ public static implicit operator Pet(Response response)
3535
{
3636
// [X] TODO: Add in HLC error semantics
3737
// [X] TODO: Use response.IsError
38-
// [ ] TODO: Use throw new ResponseFailedException(response);
38+
// [X] TODO: Use throw new ResponseFailedException(response);
39+
40+
// TODO: When we move this functionality out of Experimental into Core, it will be replaced by
41+
// > if (response.IsError)
3942
if (response.IsError())
4043
{
41-
throw new RequestFailedException("Received a non-success status code.");
44+
// TODO: When we move this functionality out of Experimental into Core, it will be replaced by
45+
// > throw new RequestFailedException(response);
46+
throw response.CreateRequestFailedException();
4247
}
4348

4449
return DeserializePet(JsonDocument.Parse(response.Content.ToMemory()));

‎sdk/core/Azure.Core.Experimental/tests/LowLevelClient/PetStoreClient.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,8 @@ public PetStoreClient(Uri endpoint, TokenCredential credential, PetStoreClientOp
5858
_tokenCredential = credential;
5959
var authPolicy = new BearerTokenAuthenticationPolicy(_tokenCredential, AuthorizationScopes);
6060

61-
// When we move the IsError functionality into Core, we'll move the addition of ResponsePropertiesPolicy to the pipeline into HttpPipelineBuilder.
62-
Pipeline = HttpPipelineBuilder.Build(options, new HttpPipelinePolicy[] { new LowLevelCallbackPolicy() }, new HttpPipelinePolicy[] { authPolicy, new ResponsePropertiesPolicy() }, new ResponseClassifier());
61+
// TODO: When we move the IsError functionality into Core, we'll move the addition of ResponsePropertiesPolicy to the pipeline into HttpPipelineBuilder.
62+
Pipeline = HttpPipelineBuilder.Build(options, new HttpPipelinePolicy[] { new LowLevelCallbackPolicy() }, new HttpPipelinePolicy[] { authPolicy, new ResponsePropertiesPolicy(options) }, new ResponseClassifier());
6363
this.endpoint = endpoint;
6464
apiVersion = options.Version;
6565
}

‎sdk/core/Azure.Core.Experimental/tests/LowLevelClientTests.cs‎

Lines changed: 65 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -91,19 +91,63 @@ public void CannotGetOutputModelOnFailureCodeAsync()
9191
Assert.ThrowsAsync<RequestFailedException>(async () => await client.GetPetAsync("pet1"));
9292
}
9393

94-
#region Helpers
94+
// Note: these two tests *currently* test different code paths:
95+
// 1) In the ResponseStatusOptions.Default case, we're going through code paths in
96+
// ClientDiagnostics that we'll later migrate to ResponseClassifier (see https://github.com/azure/azure-sdk-for-net/issues/24031)
97+
//
98+
// Because this one is thrown from client's `GetPetAsync()` method, where it calls
99+
// _clientDiagnostics.CreateRFException() -- this happens via different constructors (Note: does it have to? Could we refactor this?)
100+
//
101+
// 2) In the ResponseStatusOptions.NoThrow case, we're going through code paths in
102+
// ResponseClassifier, which will become the only path after resolution of #24031
103+
//
104+
// Importantly, having these two tests validates our premise:
105+
// ** The Grow-Up Story/HLC Helper approach has the same semantics
95106

96-
private void SerializePet(ref Utf8JsonWriter writer, Pet pet)
107+
[Test]
108+
public async Task GetRequestFailedException_StatusOptionDefault()
97109
{
98-
writer.WriteStartObject();
110+
var mockResponse = new MockResponse(404);
111+
mockResponse.SetContent("{\"error\": { \"code\": \"MockStatusCode\" }}");
112+
mockResponse.AddHeader(HttpHeader.Names.ContentType, "application/json");
99113

100-
writer.WritePropertyName("name");
101-
writer.WriteStringValue(pet.Name);
114+
var mockTransport = new MockTransport(mockResponse);
115+
PetStoreClient client = CreateClient(mockTransport);
102116

103-
writer.WritePropertyName("species");
104-
writer.WriteStringValue(pet.Species);
117+
try
118+
{
119+
Pet pet = await client.GetPetAsync("pet1");
120+
}
121+
catch (RequestFailedException e)
122+
{
123+
Assert.AreEqual(404, e.Status);
124+
Assert.That(() => e.Message.StartsWith("Service request failed."));
125+
Assert.That(() => e.ErrorCode.StartsWith("MockStatusCode"));
126+
}
127+
}
105128

106-
writer.WriteEndObject();
129+
[Test]
130+
public async Task GetRequestFailedException_StatusOptionNoThrow()
131+
{
132+
var mockResponse = new MockResponse(404);
133+
mockResponse.SetContent("{\"error\": { \"code\": \"MockStatusCode\" }}");
134+
mockResponse.AddHeader(HttpHeader.Names.ContentType, "application/json");
135+
136+
var mockTransport = new MockTransport(mockResponse);
137+
PetStoreClient client = CreateClient(mockTransport);
138+
139+
try
140+
{
141+
// NOTE: is it weird that we're saying NoThrow here and it throws?
142+
// This looks confusing to me as someone reading this code.
143+
Pet pet = await client.GetPetAsync("pet1", ResponseStatusOption.NoThrow);
144+
}
145+
catch (RequestFailedException e)
146+
{
147+
Assert.AreEqual(404, e.Status);
148+
Assert.That(() => e.Message.StartsWith("Service request failed."));
149+
Assert.That(() => e.ErrorCode.StartsWith("MockStatusCode"));
150+
}
107151
}
108152

109153
[Test]
@@ -167,6 +211,19 @@ public void ThrowOnErrorThrowsOnError()
167211
});
168212
}
169213

214+
#region Helpers
215+
private void SerializePet(ref Utf8JsonWriter writer, Pet pet)
216+
{
217+
writer.WriteStartObject();
218+
219+
writer.WritePropertyName("name");
220+
writer.WriteStringValue(pet.Name);
221+
222+
writer.WritePropertyName("species");
223+
writer.WriteStringValue(pet.Species);
224+
225+
writer.WriteEndObject();
226+
}
170227
#endregion
171228
}
172229
}

0 commit comments

Comments
 (0)