From 84e316be599ee9ae93be08b7ece086d0ff343885 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Thu, 11 Apr 2024 15:00:29 -0500 Subject: [PATCH 1/3] chore: add tsg link in error messages --- .../src/ConfigurationClient.cs | 50 +++++++++++++++++-- .../tests/ConfigurationLiveTests.cs | 3 ++ .../tests/ConfigurationMockTests.cs | 29 +++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs index 490820a617a4..c5ab515d8647 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs @@ -201,6 +201,9 @@ public virtual async Task> AddConfigurationSettin case 201: return await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false); case 412: + case 401: + case 403: + case 429: throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()); default: throw new RequestFailedException(response); @@ -240,6 +243,9 @@ public virtual Response AddConfigurationSetting(Configurat case 201: return CreateResponse(response); case 412: + case 401: + case 403: + case 429: throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()); default: throw new RequestFailedException(response); @@ -310,6 +316,9 @@ public virtual async Task> SetConfigurationSettin { 200 => await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false), 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. _ => throw new RequestFailedException(response), @@ -353,6 +362,9 @@ public virtual Response SetConfigurationSetting(Configurat { 200 => CreateResponse(response), 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. _ => throw new RequestFailedException(response), @@ -442,6 +454,9 @@ private async Task DeleteConfigurationSettingAsync(string key, string 200 => response, 204 => response, 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. _ => throw new RequestFailedException(response) @@ -471,6 +486,9 @@ private Response DeleteConfigurationSetting(string key, string label, MatchCondi 200 => response, 204 => response, 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. _ => throw new RequestFailedException(response) @@ -596,6 +614,9 @@ internal virtual async Task> GetConfigurationSett { 200 => await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false), 304 => CreateResourceModifiedResponse(response), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), _ => throw new RequestFailedException(response), }; } @@ -633,6 +654,9 @@ internal virtual Response GetConfigurationSetting(string k { 200 => CreateResponse(response), 304 => CreateResourceModifiedResponse(response), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), _ => throw new RequestFailedException(response), }; } @@ -1386,6 +1410,9 @@ private async ValueTask> SetReadOnlyAsync(string 200 => async ? await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false) : CreateResponse(response), + 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), + 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), _ => throw new RequestFailedException(response) }; } @@ -1469,16 +1496,31 @@ private static RequestContext CreateRequestContext(ErrorOptions errorOptions, Ca private class ConfigurationRequestFailedDetailsParser : RequestFailedDetailsParser { + private const string TroubleshootingText = + "For more information about this error, please see the troubleshooting guide at https://aka.ms/azsdk/net/appconfiguration/troubleshoot"; + private const string GeneralTsgSectionText = $"{TroubleshootingText}#general-troubleshooting"; + private const string LimitIssuesTroubleshootingText = $"{TroubleshootingText}#limit-issues"; + private const string AuthenticationTroubleshootingText = $"{TroubleshootingText}#troubleshooting-authentication-issues"; + private readonly Dictionary _statusCodeToErrorMessage = new() + { + { 401, AuthenticationTroubleshootingText }, + { 403, AuthenticationTroubleshootingText }, + { 409, "The setting is read only" }, + { 412, "Setting was already present." }, + { 429, LimitIssuesTroubleshootingText }, + }; public override bool TryParse(Response response, out ResponseError error, out IDictionary data) { + string errorMessage = _statusCodeToErrorMessage.TryGetValue(response.Status, out string err) ? err : GeneralTsgSectionText; + switch (response.Status) { case 409: - error = new ResponseError(null, "The setting is read only"); - data = null; - return true; case 412: - error = new ResponseError(null, "Setting was already present."); + case 401: + case 403: + case 429: + error = new ResponseError(null, errorMessage); data = null; return true; default: diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs index 93bdedc41e21..5424a14819b0 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs @@ -169,6 +169,7 @@ public async Task DeleteSetting() [RecordedTest] public async Task DeleteSettingWithLabel() { + var troubleshootingLink = "https://aka.ms/azsdk/net/appconfiguration/troubleshoot"; ConfigurationClient service = GetClient(); ConfigurationSetting testSetting = CreateSetting(); @@ -190,6 +191,8 @@ public async Task DeleteSettingWithLabel() }); Assert.AreEqual(404, e.Status); + Assert.IsNotEmpty(e.Message); + Assert.IsFalse(e.Message.Contains(troubleshootingLink)); } finally { diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs index 30e86abf2eba..936bd716db83 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs @@ -29,6 +29,7 @@ public class ConfigurationMockTests : ClientTestBase private static readonly string s_credential = "b1d9b31"; private static readonly string s_secret = "aabbccdd"; private static readonly string s_connectionString = $"Endpoint={s_endpoint};Id={s_credential};Secret={s_secret}"; + private static readonly string s_troubleshootingLink = "https://aka.ms/azsdk/net/appconfiguration/troubleshoot"; private static readonly string s_version = new ConfigurationClientOptions().Version; private static readonly ConfigurationSetting s_testSetting = new ConfigurationSetting("test_key", "test_value") @@ -108,6 +109,34 @@ public void GetNotFound() Assert.AreEqual(404, exception.Status); } + // This test validates that the client throws an exception with the expected error message when it receives a + // non-success status code from the service. + [TestCase((int)HttpStatusCode.Unauthorized, true)] + [TestCase(403, true)] + [TestCase((int)HttpStatusCode.NotFound, false)] + public void GetUnsucessfulResponse(int statusCode, bool containsTsg) + { + var response = new MockResponse(statusCode); + var mockTransport = new MockTransport(response); + ConfigurationClient service = CreateTestService(mockTransport); + + RequestFailedException exception = Assert.ThrowsAsync(async () => + { + await service.GetConfigurationSettingAsync(key: s_testSetting.Key); + }); + + Assert.AreEqual(statusCode, exception.Status); + + if (containsTsg) + { + Assert.True(exception?.Message.Contains(s_troubleshootingLink)); + } + else + { + Assert.False(exception?.Message.Contains(s_troubleshootingLink)); + } + } + [Test] public async Task GetIfChangedModified() { From a33e0577ef25a149d801d704c76b7db076774b47 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 10 May 2024 10:47:31 -0500 Subject: [PATCH 2/3] pr feedback --- .../src/ConfigurationClient.cs | 80 ++++--------------- .../tests/ConfigurationLiveTests.cs | 3 - .../tests/ConfigurationMockTests.cs | 17 ++-- 3 files changed, 21 insertions(+), 79 deletions(-) diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs index c5ab515d8647..ee1699133de6 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs @@ -200,13 +200,8 @@ public virtual async Task> AddConfigurationSettin case 200: case 201: return await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false); - case 412: - case 401: - case 403: - case 429: - throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()); default: - throw new RequestFailedException(response); + throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()); } } catch (Exception e) @@ -242,13 +237,8 @@ public virtual Response AddConfigurationSetting(Configurat case 200: case 201: return CreateResponse(response); - case 412: - case 401: - case 403: - case 429: - throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()); default: - throw new RequestFailedException(response); + throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()); } } catch (Exception e) @@ -315,10 +305,6 @@ public virtual async Task> SetConfigurationSettin return response.Status switch { 200 => await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false), - 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. _ => throw new RequestFailedException(response), @@ -361,13 +347,9 @@ public virtual Response SetConfigurationSetting(Configurat return response.Status switch { 200 => CreateResponse(response), - 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. - _ => throw new RequestFailedException(response), + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), }; } catch (Exception e) @@ -453,13 +435,9 @@ private async Task DeleteConfigurationSettingAsync(string key, string { 200 => response, 204 => response, - 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. - _ => throw new RequestFailedException(response) + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), }; } catch (Exception e) @@ -485,13 +463,9 @@ private Response DeleteConfigurationSetting(string key, string label, MatchCondi { 200 => response, 204 => response, - 409 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), // Throws on 412 if resource was modified. - _ => throw new RequestFailedException(response) + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), }; } catch (Exception e) @@ -614,10 +588,7 @@ internal virtual async Task> GetConfigurationSett { 200 => await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false), 304 => CreateResourceModifiedResponse(response), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - _ => throw new RequestFailedException(response), + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()) }; } catch (Exception e) @@ -654,10 +625,7 @@ internal virtual Response GetConfigurationSetting(string k { 200 => CreateResponse(response), 304 => CreateResourceModifiedResponse(response), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - _ => throw new RequestFailedException(response), + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()) }; } catch (Exception e) @@ -1410,10 +1378,7 @@ private async ValueTask> SetReadOnlyAsync(string 200 => async ? await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false) : CreateResponse(response), - 401 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 403 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - 429 => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), - _ => throw new RequestFailedException(response) + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), }; } catch (Exception e) @@ -1496,37 +1461,24 @@ private static RequestContext CreateRequestContext(ErrorOptions errorOptions, Ca private class ConfigurationRequestFailedDetailsParser : RequestFailedDetailsParser { - private const string TroubleshootingText = - "For more information about this error, please see the troubleshooting guide at https://aka.ms/azsdk/net/appconfiguration/troubleshoot"; - private const string GeneralTsgSectionText = $"{TroubleshootingText}#general-troubleshooting"; - private const string LimitIssuesTroubleshootingText = $"{TroubleshootingText}#limit-issues"; - private const string AuthenticationTroubleshootingText = $"{TroubleshootingText}#troubleshooting-authentication-issues"; - private readonly Dictionary _statusCodeToErrorMessage = new() - { - { 401, AuthenticationTroubleshootingText }, - { 403, AuthenticationTroubleshootingText }, - { 409, "The setting is read only" }, - { 412, "Setting was already present." }, - { 429, LimitIssuesTroubleshootingText }, - }; + private const string TroubleshootingMessage = + "For troubleshooting information, see https://aka.ms/azsdk/net/appconfiguration/troubleshoot."; public override bool TryParse(Response response, out ResponseError error, out IDictionary data) { - string errorMessage = _statusCodeToErrorMessage.TryGetValue(response.Status, out string err) ? err : GeneralTsgSectionText; - switch (response.Status) { case 409: + error = new ResponseError(null, $"The setting is read only. {TroubleshootingMessage}"); + data = null; + return true; case 412: - case 401: - case 403: - case 429: - error = new ResponseError(null, errorMessage); + error = new ResponseError(null, $"Setting was already present. {TroubleshootingMessage}"); data = null; return true; default: - error = null; + error = new ResponseError(null, TroubleshootingMessage); data = null; - return false; + return true; } } } diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs index 5424a14819b0..93bdedc41e21 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationLiveTests.cs @@ -169,7 +169,6 @@ public async Task DeleteSetting() [RecordedTest] public async Task DeleteSettingWithLabel() { - var troubleshootingLink = "https://aka.ms/azsdk/net/appconfiguration/troubleshoot"; ConfigurationClient service = GetClient(); ConfigurationSetting testSetting = CreateSetting(); @@ -191,8 +190,6 @@ public async Task DeleteSettingWithLabel() }); Assert.AreEqual(404, e.Status); - Assert.IsNotEmpty(e.Message); - Assert.IsFalse(e.Message.Contains(troubleshootingLink)); } finally { diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs index 936bd716db83..59c01dd4ee35 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/tests/ConfigurationMockTests.cs @@ -111,10 +111,10 @@ public void GetNotFound() // This test validates that the client throws an exception with the expected error message when it receives a // non-success status code from the service. - [TestCase((int)HttpStatusCode.Unauthorized, true)] - [TestCase(403, true)] - [TestCase((int)HttpStatusCode.NotFound, false)] - public void GetUnsucessfulResponse(int statusCode, bool containsTsg) + [TestCase((int)HttpStatusCode.Unauthorized)] + [TestCase(403)] + [TestCase((int)HttpStatusCode.NotFound)] + public void GetUnsucessfulResponse(int statusCode) { var response = new MockResponse(statusCode); var mockTransport = new MockTransport(response); @@ -127,14 +127,7 @@ public void GetUnsucessfulResponse(int statusCode, bool containsTsg) Assert.AreEqual(statusCode, exception.Status); - if (containsTsg) - { - Assert.True(exception?.Message.Contains(s_troubleshootingLink)); - } - else - { - Assert.False(exception?.Message.Contains(s_troubleshootingLink)); - } + Assert.True(exception?.Message.Contains(s_troubleshootingLink)); } [Test] From 7f5268b44d75657f19ea216bfc8f61736cf52213 Mon Sep 17 00:00:00 2001 From: Jorge Rangel Date: Fri, 10 May 2024 10:53:47 -0500 Subject: [PATCH 3/3] add missing error parser --- .../Azure.Data.AppConfiguration/src/ConfigurationClient.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs b/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs index ee1699133de6..82068c89e2be 100644 --- a/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs +++ b/sdk/appconfiguration/Azure.Data.AppConfiguration/src/ConfigurationClient.cs @@ -307,7 +307,7 @@ public virtual async Task> SetConfigurationSettin 200 => await CreateResponseAsync(response, cancellationToken).ConfigureAwait(false), // Throws on 412 if resource was modified. - _ => throw new RequestFailedException(response), + _ => throw new RequestFailedException(response, null, new ConfigurationRequestFailedDetailsParser()), }; } catch (Exception e)