Skip to content

Commit

Permalink
Fix bug blocking DPG and update CoreResponseClassifier constructor to…
Browse files Browse the repository at this point in the history 
… take ReadOnlySpan<ushort> (#27334)

* Fix bug blocking DPG and sm API updates

* export API

* update tests
  • Loading branch information
@annelo-msft
annelo-msft authored Mar 4, 2022
1 parent c1f475f commit 4b1550c
Show file tree
Hide file tree
Showing 10 changed files with 66 additions and 31 deletions.
2 changes: 1 addition & 1 deletion sdk/core/Azure.Core/api/Azure.Core.net461.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ public void AddPolicy(Azure.Core.Pipeline.HttpPipelinePolicy policy, Azure.Core.
}
public partial class CoreResponseClassifier : Azure.Core.ResponseClassifier
{
public CoreResponseClassifier(System.ReadOnlySpan<int> nonErrors) { }
public CoreResponseClassifier(System.ReadOnlySpan<ushort> successCodes) { }
public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; }
}
public static partial class DelegatedTokenCredential
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/Azure.Core/api/Azure.Core.net5.0.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ public void AddPolicy(Azure.Core.Pipeline.HttpPipelinePolicy policy, Azure.Core.
}
public partial class CoreResponseClassifier : Azure.Core.ResponseClassifier
{
public CoreResponseClassifier(System.ReadOnlySpan<int> nonErrors) { }
public CoreResponseClassifier(System.ReadOnlySpan<ushort> successCodes) { }
public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; }
}
public static partial class DelegatedTokenCredential
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/Azure.Core/api/Azure.Core.netcoreapp2.1.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ public void AddPolicy(Azure.Core.Pipeline.HttpPipelinePolicy policy, Azure.Core.
}
public partial class CoreResponseClassifier : Azure.Core.ResponseClassifier
{
public CoreResponseClassifier(System.ReadOnlySpan<int> nonErrors) { }
public CoreResponseClassifier(System.ReadOnlySpan<ushort> successCodes) { }
public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; }
}
public static partial class DelegatedTokenCredential
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ public void AddPolicy(Azure.Core.Pipeline.HttpPipelinePolicy policy, Azure.Core.
}
public partial class CoreResponseClassifier : Azure.Core.ResponseClassifier
{
public CoreResponseClassifier(System.ReadOnlySpan<int> nonErrors) { }
public CoreResponseClassifier(System.ReadOnlySpan<ushort> successCodes) { }
public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; }
}
public static partial class DelegatedTokenCredential
Expand Down
33 changes: 17 additions & 16 deletions sdk/core/Azure.Core/src/CoreResponseClassifier.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,30 @@ public class CoreResponseClassifier : ResponseClassifier
{
// We need 10 ulongs to represent status codes 100 - 599.
private const int Length = 10;
private ulong[] _nonErrors;
private ulong[] _successCodes;

internal ResponseClassificationHandler[]? Handlers { get; set; }

/// <summary>
/// Creates a new instance of <see cref="CoreResponseClassifier"/>
/// </summary>
/// <param name="nonErrors"></param>
public CoreResponseClassifier(ReadOnlySpan<int> nonErrors)
/// <param name="successCodes">The status codes that this classifier will consider
/// not to be errors.</param>
public CoreResponseClassifier(ReadOnlySpan<ushort> successCodes)
{
_nonErrors = new ulong[Length];
_successCodes = new ulong[Length];

foreach (int statusCode in nonErrors)
foreach (int statusCode in successCodes)
{
AddClassifier(statusCode, isError: false);
}
}

private CoreResponseClassifier(ulong[] nonErrors, ResponseClassificationHandler[]? handlers)
private CoreResponseClassifier(ulong[] successCodes, ResponseClassificationHandler[]? handlers)
{
Debug.Assert(nonErrors?.Length == Length);
Debug.Assert(successCodes?.Length == Length);

_nonErrors = nonErrors!;
_successCodes = successCodes!;
Handlers = handlers;
}

Expand All @@ -58,15 +59,15 @@ public override bool IsErrorResponse(HttpMessage message)
}
}

return !IsNonError(message.Response.Status);
return !IsSuccessCode(message.Response.Status);
}

internal virtual CoreResponseClassifier Clone()
{
ulong[] nonErrors = new ulong[Length];
Array.Copy(_nonErrors, nonErrors, Length);
ulong[] successCodes = new ulong[Length];
Array.Copy(_successCodes, successCodes, Length);

return new CoreResponseClassifier(nonErrors, Handlers);
return new CoreResponseClassifier(successCodes, Handlers);
}

internal void AddClassifier(int statusCode, bool isError)
Expand All @@ -77,7 +78,7 @@ internal void AddClassifier(int statusCode, bool isError)
int bit = statusCode & 0b111111; // keeps the bits up to 63
ulong mask = 1ul << bit; // shifts a 1 to the position of code

ulong value = _nonErrors[index];
ulong value = _successCodes[index];
if (!isError)
{
value |= mask;
Expand All @@ -87,16 +88,16 @@ internal void AddClassifier(int statusCode, bool isError)
value &= ~mask;
}

_nonErrors[index] = value;
_successCodes[index] = value;
}

private bool IsNonError(int statusCode)
private bool IsSuccessCode(int statusCode)
{
var index = statusCode >> 6; // divides by 64
int bit = statusCode & 0b111111; // keeps the bits up to 63
ulong mask = 1ul << bit; // shifts a 1 to the position of code

ulong value = _nonErrors[index];
ulong value = _successCodes[index];
return (value & mask) != 0;
}
}
Expand Down
4 changes: 4 additions & 0 deletions sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ public HttpMessage CreateMessage()
public HttpMessage CreateMessage(RequestContext? context, ResponseClassifier? classifier = default)
{
var message = CreateMessage();
if (classifier != null)
{
message.ResponseClassifier = classifier;
}
message.ApplyRequestContext(context, classifier);
return message;
}
Expand Down
6 changes: 3 additions & 3 deletions sdk/core/Azure.Core/tests/CoreResponseClassifierTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ internal class CoreResponseClassifierTests
public void ClassifiesSingleCodeAsNonError()
{
// test classifiers for each of the status codes
for (int nonError = 100; nonError <= 599; nonError++)
for (ushort nonError = 100; nonError <= 599; nonError++)
{
CoreResponseClassifier classifier = new CoreResponseClassifier(new int[] { nonError });
CoreResponseClassifier classifier = new CoreResponseClassifier(new ushort[] { nonError });
HttpMessage message = new HttpMessage(new MockRequest(), classifier);

// test all the status codes against the classifier
Expand Down Expand Up @@ -43,7 +43,7 @@ public void ClassifiesSingleCodeAsNonError()
[TestCase(502, true)]
public void ClassifiesMultipleCodesAsNonErrors(int code, bool isError)
{
CoreResponseClassifier classifier = new CoreResponseClassifier(stackalloc int[] { 200, 404 });
CoreResponseClassifier classifier = new CoreResponseClassifier(stackalloc ushort[] { 200, 404 });

HttpMessage message = new HttpMessage(new MockRequest(), classifier);

Expand Down
10 changes: 5 additions & 5 deletions sdk/core/Azure.Core/tests/HttpMessageTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public void SettingResponseClassifierReplacesBaseClassifier()

// This replaces the base classifier with one that thinks
// only 404 is a non-error.
message.ResponseClassifier = new CoreResponseClassifier(stackalloc int[] { 404 });
message.ResponseClassifier = new CoreResponseClassifier(stackalloc ushort[] { 404 });

message.Response = new MockResponse(204);
Assert.IsTrue(message.ResponseClassifier.IsErrorResponse(message));
Expand Down Expand Up @@ -147,7 +147,7 @@ public void SettingResponseClassifierReplacesBaseClassifier_PerCallCustomization

// This replaces the base classifier with one that only thinks 404 is a non-error
// and doesn't have opinions on anything else.
message.ResponseClassifier = new CoreResponseClassifier(stackalloc int[] { 404 });
message.ResponseClassifier = new CoreResponseClassifier(stackalloc ushort[] { 404 });

message.Response = new MockResponse(304);
Assert.IsTrue(message.ResponseClassifier.IsErrorResponse(message));
Expand All @@ -166,7 +166,7 @@ public void SettingResponseClassifierReplacesBaseClassifier_PerClientCustomizati
ClientOptions.Default,
new HttpPipelinePolicy[] { },
new HttpPipelinePolicy[] { },
new CoreResponseClassifier(stackalloc int[] { 404 }));
new CoreResponseClassifier(stackalloc ushort[] { 404 }));

var message = pipeline.CreateMessage();

Expand All @@ -176,7 +176,7 @@ public void SettingResponseClassifierReplacesBaseClassifier_PerClientCustomizati
message.Response = new MockResponse(404);
Assert.IsFalse(message.ResponseClassifier.IsErrorResponse(message));

message.ResponseClassifier = new CoreResponseClassifier(stackalloc int[] { 304 });
message.ResponseClassifier = new CoreResponseClassifier(stackalloc ushort[] { 304 });

message.Response = new MockResponse(304);
Assert.IsFalse(message.ResponseClassifier.IsErrorResponse(message));
Expand Down Expand Up @@ -357,7 +357,7 @@ public override bool TryClassify(HttpMessage message, out bool isError)

// Example DPG classifier for testing purposes.
private static ResponseClassifier _responseClassifier200204304;
private static ResponseClassifier ResponseClassifier200204304 => _responseClassifier200204304 ??= new CoreResponseClassifier(stackalloc int[] { 200, 204, 304 });
private static ResponseClassifier ResponseClassifier200204304 => _responseClassifier200204304 ??= new CoreResponseClassifier(stackalloc ushort[] { 200, 204, 304 });

private sealed class HeadResponseClassifier : ResponseClassifier
{
Expand Down
32 changes: 31 additions & 1 deletion sdk/core/Azure.Core/tests/HttpPipelineTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,36 @@ public async Task RequestContextClassifierSetsResponseIsError()
Assert.IsFalse(response.IsError);
}

[Test]
[TestCase(100, true)]
[TestCase(200, false)]
[TestCase(201, true)]
[TestCase(202, true)]
[TestCase(204, false)]
[TestCase(300, true)]
[TestCase(304, false)]
[TestCase(400, true)]
[TestCase(404, true)]
[TestCase(500, true)]
[TestCase(504, true)]
public async Task RequestContextDefault_IsErrorIsSet(int code, bool isError)
{
var mockTransport = new MockTransport(
new MockResponse(code));

var pipeline = new HttpPipeline(mockTransport, default);

HttpMessage message = pipeline.CreateMessage(context: default, ResponseClassifier200204304);
Request request = message.Request;
request.Method = RequestMethod.Get;
request.Uri.Reset(new Uri("https://contoso.a.io"));

await pipeline.SendAsync(message, CancellationToken.None);
Response response = message.Response;

Assert.AreEqual(isError, response.IsError);
}

#region Helpers
public class AddHeaderPolicy : HttpPipelineSynchronousPolicy
{
Expand Down Expand Up @@ -346,7 +376,7 @@ public override bool IsErrorResponse(HttpMessage message)

// How classifiers will be generated in DPG.
private static ResponseClassifier _responseClassifier200204304;
private static ResponseClassifier ResponseClassifier200204304 => _responseClassifier200204304 ??= new CoreResponseClassifier(stackalloc int[] { 200, 204, 304 });
private static ResponseClassifier ResponseClassifier200204304 => _responseClassifier200204304 ??= new CoreResponseClassifier(stackalloc ushort[] { 200, 204, 304 });
#endregion

}
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/Azure.Core/tests/RequestContextTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ public async Task CanSuppressLoggingAsError()
var response = new MockResponse(404);
var mockTransport = new MockTransport(response);

var classifier = new CoreResponseClassifier(stackalloc int[] { 200, 204, 304 });
var classifier = new CoreResponseClassifier(stackalloc ushort[] { 200, 204, 304 });
var pipeline = new HttpPipeline(mockTransport, new[] { new LoggingPolicy(logContent: true, int.MaxValue, HttpMessageSanitizer.Default, "Test SDK") });

var context = new RequestContext();
Expand Down Expand Up @@ -262,7 +262,7 @@ public async Task CanSuppressTracingAsError()
return mockResponse;
});

var classifier = new CoreResponseClassifier(stackalloc int[] { 200, 204, 304 });
var classifier = new CoreResponseClassifier(stackalloc ushort[] { 200, 204, 304 });
var pipeline = new HttpPipeline(mockTransport, new[] { new RequestActivityPolicy(true, "Azure.Core.Tests", HttpMessageSanitizer.Default) });
var context = new RequestContext();
context.AddClassifier(409, isError: false);
Expand Down

0 comments on commit 4b1550c

Please sign in to comment.