-
Notifications
You must be signed in to change notification settings - Fork 29
(bunq/sdk_csharp#63) add response id to failed request #73
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
Changes from 17 commits
2fcf849
811b35d
aff6b20
03912ab
60e18a5
e775766
6eddd49
961715a
4a42f7d
e59fcf3
3d9c9de
3d7bb5e
3698815
7429ce0
7cb55ae
29daba5
e88274c
5a2a805
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| using System; | ||
| using Bunq.Sdk.Context; | ||
| using Bunq.Sdk.Exception; | ||
| using Bunq.Sdk.Model.Generated.Endpoint; | ||
| using Xunit; | ||
|
|
||
| namespace Bunq.Sdk.Tests.Http | ||
| { | ||
| public class ResponseIdOnBadRequestTest : BunqSdkTestBase | ||
| { | ||
| /// <summary> | ||
| /// API context to use for the test API calls. | ||
| /// </summary> | ||
| private static readonly ApiContext API_CONTEXT = GetApiContext(); | ||
|
|
||
| /// <summary> | ||
| /// Invalid user id to trigger BadRequestException | ||
| /// </summary> | ||
| private const int INVALID_USER_PERSON_ID = 0; | ||
|
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. UpperCamelCase 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 will be refactored in #58 |
||
|
|
||
| [Fact] | ||
| public void TestBadRequestWithResponseId() | ||
| { | ||
| var caughtException = Assert.Throws<BadRequestException>( | ||
| () => UserPerson.Get(API_CONTEXT, INVALID_USER_PERSON_ID) | ||
| ); | ||
|
|
||
| Assert.NotNull(caughtException.ResponseId); | ||
| } | ||
| } | ||
| } | ||
|
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. Newline at EOF |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| using System.Collections.Generic; | ||
| using System; | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace Bunq.Sdk.Exception | ||
| { | ||
| /// <summary> | ||
| /// This class makes sure that the correct exception is thrown for the given response code. | ||
| /// </summary> | ||
| public class ExceptionFactory | ||
| { | ||
| /// <summary> | ||
|
|
@@ -16,39 +20,49 @@ public class ExceptionFactory | |
| private const int HTTP_RESPONSE_CODE_INTERNAL_SERVER_ERROR = 500; | ||
|
|
||
| /// <summary> | ||
| /// Glue to concatenate the error messages. | ||
| /// String format constants. | ||
| /// </summary> | ||
| private const string GLUE_ERROR_MESSAGES = "\n"; | ||
| private const string FORMAT_ERROR_MESSAGE = "Response id to help bunq debug: {0}. \n Error message: {1}"; | ||
|
|
||
| /// <returns>The exception that belongs to this status code.</returns> | ||
| public static ApiException CreateExceptionForResponse(int responseCode, IList<string> messages) | ||
| public static ApiException CreateExceptionForResponse( | ||
| int responseCode, | ||
| IEnumerable<string> messages, | ||
| string responseId | ||
| ) | ||
| { | ||
|
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. Shouldn't this open brace go on the same line as the close parenthese? 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. it is ? 😁 |
||
| var errorMessage = ConcatenateMessages(messages); | ||
| var errorMessage = FormatExceptionMessage(messages, responseId); | ||
|
|
||
| switch (responseCode) | ||
| { | ||
| case HTTP_RESPONSE_CODE_BAD_REQUEST: | ||
| return new BadRequestException(responseCode, errorMessage); | ||
| return new BadRequestException(responseCode, errorMessage, responseId); | ||
| case HTTP_RESPONSE_CODE_UNAUTHORIZED: | ||
| return new UnauthorizedException(responseCode, errorMessage); | ||
| return new UnauthorizedException(responseCode, errorMessage, responseId); | ||
| case HTTP_RESPONSE_CODE_FORBIDDEN: | ||
| return new ForbiddenException(responseCode, errorMessage); | ||
| return new ForbiddenException(responseCode, errorMessage, responseId); | ||
| case HTTP_RESPONSE_CODE_NOT_FOUND: | ||
| return new NotFoundException(responseCode, errorMessage); | ||
| return new NotFoundException(responseCode, errorMessage, responseId); | ||
| case HTTP_RESPONSE_CODE_METHOD_NOT_ALLOWED: | ||
| return new MethodNotAllowedException(responseCode, errorMessage); | ||
| return new MethodNotAllowedException(responseCode, errorMessage, responseId); | ||
| case HTTP_RESPONSE_CODE_TOO_MANY_REQUESTS: | ||
| return new TooManyRequestsException(responseCode, errorMessage); | ||
| return new TooManyRequestsException(responseCode, errorMessage, responseId); | ||
| case HTTP_RESPONSE_CODE_INTERNAL_SERVER_ERROR: | ||
| return new PleaseContactBunqException(responseCode, errorMessage); | ||
| return new PleaseContactBunqException(responseCode, errorMessage, responseId); | ||
| default: | ||
| return new UnknownApiErrorException(responseCode, errorMessage); | ||
| return new UnknownApiErrorException(responseCode, errorMessage, responseId); | ||
| } | ||
| } | ||
|
|
||
| private static string ConcatenateMessages(IEnumerable<string> messages) | ||
| /// <summary> | ||
| /// Formats the exception message accordingly. | ||
| /// </summary> | ||
| private static string FormatExceptionMessage(IEnumerable<string> messages, string responseId) | ||
| { | ||
| return string.Join(GLUE_ERROR_MESSAGES, messages); | ||
| return string.Format(FORMAT_ERROR_MESSAGE, | ||
| responseId, | ||
| string.Join(Environment.NewLine, messages) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,8 @@ | |
| { | ||
| public class ForbiddenException : ApiException | ||
|
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. Docs Same for other exception types 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 code has not been modified in this pr. Please create a follow up issue for this. This would need to be refactored in the other SDK's as well. |
||
| { | ||
| public ForbiddenException(int responseCode, string message) : base(responseCode, message) | ||
| public ForbiddenException(int responseCode, string message, string responseId) | ||
| : base(responseCode, message, responseId) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,12 @@ namespace Bunq.Sdk.Http | |
| { | ||
| public class ApiClient | ||
| { | ||
|
|
||
| /// <summary> | ||
| /// Error constatns. | ||
| /// </summary> | ||
| private static string ERROR_COULD_NOT_DETERMINE_RESPONSE_ID_HEADER = | ||
|
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. UpperCamelCase 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. Will be refactored in #58 |
||
| "The response header \"X-Bunq-Client-Response-Id\" or \"x-bunq-client-response-id\" could not be found."; | ||
|
|
||
| /// <summary> | ||
| /// Endpoints not requiring active session for the request to succeed. | ||
|
|
@@ -43,6 +49,8 @@ public class ApiClient | |
| private const string HEADER_GEOLOCATION = "X-Bunq-Geolocation"; | ||
| private const string HEADER_SIGNATURE = "X-Bunq-Client-Signature"; | ||
| private const string HEADER_AUTHENTICATION = "X-Bunq-Client-Authentication"; | ||
| private static string HEADER_RESPONSE_ID_LOWER_CASE = "x-bunq-client-response-id"; | ||
|
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. UpperCamelCase 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. Will be refactored in #58 |
||
| private static string HEADER_RESPONSE_ID_UPPER_CASE = "X-Bunq-Client-Response-Id"; | ||
|
|
||
| /// <summary> | ||
| /// Field constants. | ||
|
|
@@ -282,18 +290,50 @@ private static void AssertResponseSuccess(HttpResponseMessage responseMessage) | |
| var responseCode = (int) responseMessage.StatusCode; | ||
| var responseBody = responseMessage.Content.ReadAsStringAsync().Result; | ||
|
|
||
| throw CreateApiExceptionRequestUnsuccessful(responseCode, responseBody); | ||
| throw CreateApiExceptionRequestUnsuccessful( | ||
| responseCode, | ||
| responseBody, | ||
| DetermineResponseIdByAllHeader(responseMessage.Headers) | ||
| ); | ||
| } | ||
|
|
||
| private static string DetermineResponseIdByAllHeader(HttpHeaders allHeader) | ||
| { | ||
| if (allHeader.Contains(HEADER_RESPONSE_ID_UPPER_CASE)) | ||
|
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. Maybe a method could be extracted here to avoid the conditional expressions. 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 cant seem to picture how an extracted method will prevent these conditional lines 🤔. Could you please provide a snippet with what you mean ? 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. @OGKevin something like: private static string GetResponseId(HttpHeaders allHeader, IEnumerable<string> allHeaderResponse)
{
foreach (var headerResponse in allHeaderResponse)
{
if (allHeader.Contains(headerResponse))
{
return allHeader.GetValues(headerResponse).First();
}
}
throw new BunqException(ERROR_COULD_NOT_DETERMINE_RESPONSE_ID_HEADER);
}Actually... the loop brings needed extra complexity, in this case. Maybe it is better to stick with the conditional statements. 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. Yes I think it is indeed better if we stick to the conditional statements! 👍 |
||
| { | ||
| return allHeader.GetValues(HEADER_RESPONSE_ID_UPPER_CASE).First(); | ||
| } | ||
| else if (allHeader.Contains(HEADER_RESPONSE_ID_LOWER_CASE)) | ||
| { | ||
| return allHeader.GetValues(HEADER_RESPONSE_ID_LOWER_CASE).First(); | ||
| } | ||
|
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. missing |
||
| else | ||
| { | ||
| throw new BunqException(ERROR_COULD_NOT_DETERMINE_RESPONSE_ID_HEADER); | ||
| } | ||
| } | ||
|
|
||
| private static ApiException CreateApiExceptionRequestUnsuccessful(int responseCode, string responseBody) | ||
| private static ApiException CreateApiExceptionRequestUnsuccessful( | ||
| int responseCode, | ||
| string responseBody, | ||
| string responseId | ||
| ) | ||
| { | ||
| try | ||
| { | ||
| return ExceptionFactory.CreateExceptionForResponse(responseCode, FetchErrorDescriptions(responseBody)); | ||
| return ExceptionFactory.CreateExceptionForResponse( | ||
| responseCode, | ||
| FetchErrorDescriptions(responseBody), | ||
| responseId | ||
| ); | ||
| } | ||
| catch (JsonException) | ||
| { | ||
| return ExceptionFactory.CreateExceptionForResponse(responseCode, new List<string> {responseBody}); | ||
| return ExceptionFactory.CreateExceptionForResponse( | ||
| responseCode, | ||
| new List<string> {responseBody}, | ||
| responseId | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
UpperCamelCase
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 will be refactored in #58