From 4b1550c21cc31c654681beb20a02a7e7afc23aba Mon Sep 17 00:00:00 2001 From: Anne Thompson Date: Fri, 4 Mar 2022 13:03:13 -0800 Subject: [PATCH] Fix bug blocking DPG and update CoreResponseClassifier constructor to take ReadOnlySpan (#27334) * Fix bug blocking DPG and sm API updates * export API * update tests --- sdk/core/Azure.Core/api/Azure.Core.net461.cs | 2 +- sdk/core/Azure.Core/api/Azure.Core.net5.0.cs | 2 +- .../api/Azure.Core.netcoreapp2.1.cs | 2 +- .../api/Azure.Core.netstandard2.0.cs | 2 +- .../Azure.Core/src/CoreResponseClassifier.cs | 33 ++++++++++--------- .../Azure.Core/src/Pipeline/HttpPipeline.cs | 4 +++ .../tests/CoreResponseClassifierTests.cs | 6 ++-- sdk/core/Azure.Core/tests/HttpMessageTests.cs | 10 +++--- .../Azure.Core/tests/HttpPipelineTests.cs | 32 +++++++++++++++++- .../Azure.Core/tests/RequestContextTests.cs | 4 +-- 10 files changed, 66 insertions(+), 31 deletions(-) diff --git a/sdk/core/Azure.Core/api/Azure.Core.net461.cs b/sdk/core/Azure.Core/api/Azure.Core.net461.cs index b6bbf0ce01f2..1715cbea1e35 100644 --- a/sdk/core/Azure.Core/api/Azure.Core.net461.cs +++ b/sdk/core/Azure.Core/api/Azure.Core.net461.cs @@ -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 nonErrors) { } + public CoreResponseClassifier(System.ReadOnlySpan successCodes) { } public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; } } public static partial class DelegatedTokenCredential diff --git a/sdk/core/Azure.Core/api/Azure.Core.net5.0.cs b/sdk/core/Azure.Core/api/Azure.Core.net5.0.cs index 757ca97db95d..54009043f9cf 100644 --- a/sdk/core/Azure.Core/api/Azure.Core.net5.0.cs +++ b/sdk/core/Azure.Core/api/Azure.Core.net5.0.cs @@ -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 nonErrors) { } + public CoreResponseClassifier(System.ReadOnlySpan successCodes) { } public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; } } public static partial class DelegatedTokenCredential diff --git a/sdk/core/Azure.Core/api/Azure.Core.netcoreapp2.1.cs b/sdk/core/Azure.Core/api/Azure.Core.netcoreapp2.1.cs index b6bbf0ce01f2..1715cbea1e35 100644 --- a/sdk/core/Azure.Core/api/Azure.Core.netcoreapp2.1.cs +++ b/sdk/core/Azure.Core/api/Azure.Core.netcoreapp2.1.cs @@ -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 nonErrors) { } + public CoreResponseClassifier(System.ReadOnlySpan successCodes) { } public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; } } public static partial class DelegatedTokenCredential diff --git a/sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs b/sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs index b6bbf0ce01f2..1715cbea1e35 100644 --- a/sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs +++ b/sdk/core/Azure.Core/api/Azure.Core.netstandard2.0.cs @@ -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 nonErrors) { } + public CoreResponseClassifier(System.ReadOnlySpan successCodes) { } public override bool IsErrorResponse(Azure.Core.HttpMessage message) { throw null; } } public static partial class DelegatedTokenCredential diff --git a/sdk/core/Azure.Core/src/CoreResponseClassifier.cs b/sdk/core/Azure.Core/src/CoreResponseClassifier.cs index 3969c09f6e19..231013377333 100644 --- a/sdk/core/Azure.Core/src/CoreResponseClassifier.cs +++ b/sdk/core/Azure.Core/src/CoreResponseClassifier.cs @@ -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; } /// /// Creates a new instance of /// - /// - public CoreResponseClassifier(ReadOnlySpan nonErrors) + /// The status codes that this classifier will consider + /// not to be errors. + public CoreResponseClassifier(ReadOnlySpan 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; } @@ -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) @@ -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; @@ -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; } } diff --git a/sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs b/sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs index b42165e5fe61..d9a9b9eea4c1 100644 --- a/sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs +++ b/sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs @@ -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; } diff --git a/sdk/core/Azure.Core/tests/CoreResponseClassifierTests.cs b/sdk/core/Azure.Core/tests/CoreResponseClassifierTests.cs index 78cd9b35641c..31ec03c2249e 100644 --- a/sdk/core/Azure.Core/tests/CoreResponseClassifierTests.cs +++ b/sdk/core/Azure.Core/tests/CoreResponseClassifierTests.cs @@ -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 @@ -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); diff --git a/sdk/core/Azure.Core/tests/HttpMessageTests.cs b/sdk/core/Azure.Core/tests/HttpMessageTests.cs index 82c98aa00583..0eb60f2eba29 100644 --- a/sdk/core/Azure.Core/tests/HttpMessageTests.cs +++ b/sdk/core/Azure.Core/tests/HttpMessageTests.cs @@ -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)); @@ -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)); @@ -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(); @@ -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)); @@ -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 { diff --git a/sdk/core/Azure.Core/tests/HttpPipelineTests.cs b/sdk/core/Azure.Core/tests/HttpPipelineTests.cs index 2dac0be6f421..7e03d1c73bef 100644 --- a/sdk/core/Azure.Core/tests/HttpPipelineTests.cs +++ b/sdk/core/Azure.Core/tests/HttpPipelineTests.cs @@ -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 { @@ -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 } diff --git a/sdk/core/Azure.Core/tests/RequestContextTests.cs b/sdk/core/Azure.Core/tests/RequestContextTests.cs index d63c53c5c927..af4771a00325 100644 --- a/sdk/core/Azure.Core/tests/RequestContextTests.cs +++ b/sdk/core/Azure.Core/tests/RequestContextTests.cs @@ -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(); @@ -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);