-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Introduce AbuseException #1528
Introduce AbuseException #1528
Changes from 27 commits
b3c8b64
b3e85f2
56f853e
f9bbceb
8a915b2
8095d10
4283923
abf73a4
22524f9
417133f
496ee8b
a5f3f90
048eca7
e4f7f90
bfedee2
51b0e0d
16d8975
e1eaac1
bba1644
a0c5747
d131b37
88f63ad
01d203a
2acb69e
5edeac4
ba8fbfe
47126c9
04b9475
ffb7d6a
9010f0d
507d2cb
a9b9437
fa7ffc7
4076e08
6927842
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
using System.Collections.Generic; | ||
using System.Net; | ||
using Octokit.Internal; | ||
using Xunit; | ||
|
||
namespace Octokit.Tests.Exceptions | ||
{ | ||
public class AbuseExceptionTests | ||
{ | ||
[Fact] | ||
public void PostsAbuseMessageFromApi() | ||
{ | ||
|
||
const string responseBody = "{\"message\":\"You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.\"," + | ||
"\"documentation_url\":\"https://developer.github.com/v3/#abuse-rate-limits\"}"; | ||
|
||
var response = new Response( | ||
HttpStatusCode.Forbidden, | ||
responseBody, | ||
new Dictionary<string, string>(), | ||
"application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.Equal("You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.", abuseException.ApiError.Message); | ||
} | ||
|
||
[Fact] | ||
public void HasDefaultMessage() | ||
{ | ||
var response = new Response(HttpStatusCode.Forbidden, null, new Dictionary<string, string>(), "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.Equal("Request Forbidden - Abuse Detection", abuseException.Message); | ||
} | ||
|
||
public class RetryAfterHeaderHandling | ||
{ | ||
[Fact] | ||
public void WithRetryAfterHeader_PopulatesRetryAfterSeconds() | ||
{ | ||
var headerDictionary = new Dictionary<string, string> | ||
{ | ||
{ "Retry-After", "30" } | ||
}; | ||
|
||
var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.Equal(30, abuseException.RetryAfterSeconds); | ||
} | ||
|
||
[Fact] | ||
public void NoRetryAfterHeader_RetryAfterSecondsIsSetToTheDefaultOfNull() | ||
{ | ||
var headerDictionary = new Dictionary<string, string>(); | ||
|
||
var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.False(abuseException.RetryAfterSeconds.HasValue); | ||
} | ||
|
||
[Theory] | ||
[InlineData(null)] | ||
[InlineData("")] | ||
[InlineData(" ")] | ||
public void EmptyHeaderValue_RetryAfterSecondsDefaultsToNull(string emptyValueToTry) | ||
{ | ||
var headerDictionary = new Dictionary<string, string> | ||
{ | ||
{ "Retry-After", emptyValueToTry } | ||
}; | ||
|
||
var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.False(abuseException.RetryAfterSeconds.HasValue); | ||
} | ||
|
||
[Fact] | ||
public void NonParseableIntHeaderValue_RetryAfterSecondsDefaultsToNull() | ||
{ | ||
var headerDictionary = new Dictionary<string, string> | ||
{ | ||
{ "Retry-After", "ABC" } | ||
}; | ||
|
||
var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.False(abuseException.RetryAfterSeconds.HasValue); | ||
} | ||
|
||
[Fact] | ||
public void NegativeHeaderValue_RetryAfterSecondsDefaultsToNull() | ||
{ | ||
var headerDictionary = new Dictionary<string, string> | ||
{ | ||
{ "Retry-After", "-123" } | ||
}; | ||
|
||
var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.False(abuseException.RetryAfterSeconds.HasValue); | ||
} | ||
|
||
[Fact] | ||
public void ZeroHeaderValue_RetryAfterSecondsIsZero() | ||
{ | ||
var headerDictionary = new Dictionary<string, string> | ||
{ | ||
{ "Retry-After", "0" } | ||
}; | ||
|
||
var response = new Response(HttpStatusCode.Forbidden, null, headerDictionary, "application/json"); | ||
var abuseException = new AbuseException(response); | ||
|
||
Assert.Equal(0, abuseException.RetryAfterSeconds); | ||
} | ||
|
||
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. Extra whitespace line |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
using System; | ||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Net; | ||
using System.Runtime.Serialization; | ||
using System.Security; | ||
|
||
namespace Octokit | ||
{ | ||
/// <summary> | ||
/// Represents a subset of the HTTP 403 - Forbidden response returned from the API when the forbidden response is related to an abuse detection mechanism. | ||
/// Containts the amount of seconds after which it's safe to retry the request. | ||
/// </summary> | ||
#if !NETFX_CORE | ||
[Serializable] | ||
#endif | ||
[SuppressMessage("Microsoft.Design", "CA1032:ImplementStandardExceptionConstructors", | ||
Justification = "These exceptions are specific to the GitHub API and not general purpose exceptions")] | ||
public class AbuseException : ForbiddenException | ||
{ | ||
private readonly int? RetrySecondsDefault = null; | ||
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 a whitespace line after this line |
||
/// <summary> | ||
/// Constructs an instance of AbuseException | ||
/// </summary> | ||
/// <param name="response">The HTTP payload from the server</param> | ||
public AbuseException(IResponse response) : this(response, null) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// Constructs an instance of AbuseException | ||
/// </summary> | ||
/// <param name="response">The HTTP payload from the server</param> | ||
/// <param name="innerException">The inner exception</param> | ||
public AbuseException(IResponse response, Exception innerException) | ||
: base(response, innerException) | ||
{ | ||
Debug.Assert(response != null && response.StatusCode == HttpStatusCode.Forbidden, | ||
"AbuseException created with wrong status code"); | ||
|
||
SetRetryAfterSeconds(response); | ||
} | ||
|
||
private void SetRetryAfterSeconds(IResponse response) | ||
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. To me it seems a bit overkill to have a property, default property and 2 private functions, all related to pulling the value out of the headers and parsing it. I think maintainability would be higher if it was a single function called from the Im happy to discuss if you feel things are more granularly testable with the current implementation but this was my initial thought around maintainability/readability... 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. @ryangribble sure, makes sense. I'll clean it up and compact it so you can take a second pass. |
||
{ | ||
string secondsValue; | ||
|
||
// ReSharper disable once ConvertIfStatementToConditionalTernaryExpression | ||
if (response.Headers.TryGetValue("Retry-After", out secondsValue)) | ||
{ | ||
RetryAfterSeconds = ParseRetryAfterSeconds(secondsValue); | ||
} | ||
|
||
else | ||
{ | ||
RetryAfterSeconds = RetrySecondsDefault; | ||
} | ||
} | ||
|
||
private int? ParseRetryAfterSeconds(string retryAfterString) | ||
{ | ||
if (string.IsNullOrWhiteSpace(retryAfterString)) | ||
{ | ||
return RetrySecondsDefault; | ||
} | ||
|
||
int retrySeconds; | ||
if (int.TryParse(retryAfterString, out retrySeconds)) | ||
{ | ||
return retrySeconds < 0 ? RetrySecondsDefault : retrySeconds; | ||
} | ||
|
||
return RetrySecondsDefault; | ||
} | ||
|
||
public int? RetryAfterSeconds { get; private set; } | ||
|
||
public override string Message | ||
{ | ||
get { return ApiErrorMessageSafe ?? "Request Forbidden - Abuse Detection"; } | ||
} | ||
|
||
#if !NETFX_CORE | ||
/// <summary> | ||
/// Constructs an instance of AbuseException | ||
/// </summary> | ||
/// <param name="info"> | ||
/// The <see cref="SerializationInfo"/> that holds the | ||
/// serialized object data about the exception being thrown. | ||
/// </param> | ||
/// <param name="context"> | ||
/// The <see cref="StreamingContext"/> that contains | ||
/// contextual information about the source or destination. | ||
/// </param> | ||
protected AbuseException(SerializationInfo info, StreamingContext context) | ||
: base(info, context) | ||
{ | ||
} | ||
|
||
[SecurityCritical] | ||
public override void GetObjectData(SerializationInfo info, StreamingContext context) | ||
{ | ||
base.GetObjectData(info, context); | ||
info.AddValue("RetryAfterSeconds", RetryAfterSeconds); | ||
} | ||
|
||
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. Extra whitespace line |
||
#endif | ||
} | ||
} |
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.
The style/verbage used in the naming of the test fixtures isn't too consistent with (most) of the other octokit tests (or more precisely, the direction/consistency we are meandering towards)
We normally word the test fixture naming with sub classes for the functions eg
TheConstructor
TheParseRetryHeaderMethod
etc, and then the name of the[Fixture]
test functions is likeHandlesEmptyHeader
HandlesNullHeader
CorrectlyParsesValue
and so on.So that way we end up with common english sounding
AbuseExceptionTests=>TheConstructor=>CorrectlyDeterminesRetryAfterValue
and so on
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.
@ryangribble makes sense. I'll reformat. Sorry, should've paid more attention to the existing conventions. They tend to be my preference anyway; I think I was likely being hasty.
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.
We have a fairly large code base and some of it has not needed to be touched for quite a while, so it's certainly not all consistent! As we go forward (and as we work with new contributors) we are taking a greater focus on ensuring consistency at least in anything new we implement, so thanks for jumping through these hoops with me 👍