-
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
Merged
Merged
Introduce AbuseException #1528
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
b3c8b64
Merge remote-tracking branch 'refs/remotes/octokit/master'
SeanKilleen b3e85f2
Add some tests to be completed
SeanKilleen 56f853e
Actually fail the tests
SeanKilleen f9bbceb
Create AbuseException class
SeanKilleen 8a915b2
Actually add AbuseException to csproj
SeanKilleen 8095d10
Update test file
SeanKilleen 4283923
Ran .\build FixProjects
SeanKilleen abf73a4
Test updates
SeanKilleen 22524f9
Default message update for AbuseException
SeanKilleen 417133f
Separate the exception creation logic
SeanKilleen 496ee8b
Remove message assertion -- doesn't matter here
SeanKilleen a5f3f90
Additional test for abuse message
SeanKilleen 048eca7
Remove unnecessary variable assignment
SeanKilleen e4f7f90
Failing test for unsafe request
SeanKilleen bfedee2
Attempt to fix test
SeanKilleen 51b0e0d
Remove test that will always fail due to another issue
SeanKilleen 16d8975
New tests (some failing)
SeanKilleen e1eaac1
Passing tests are, like, better than failing tests.
SeanKilleen bba1644
Last passing test
SeanKilleen a0c5747
Cleanup
SeanKilleen d131b37
Add test for zero value and fix code
SeanKilleen 88f63ad
cleanup
SeanKilleen 01d203a
Mark ParseRetryAfterSeconds as static
SeanKilleen 2acb69e
Add GetObjectData override to AbuseException
SeanKilleen 5edeac4
Add back failing test & skip it
SeanKilleen ba8fbfe
Change to nullable int with null default
SeanKilleen 47126c9
Fix tests around nullable default
SeanKilleen 04b9475
Merge remote-tracking branch 'refs/remotes/octokit/master' into Add-A…
SeanKilleen ffb7d6a
whitespace fixes
SeanKilleen 9010f0d
Compact the logic; tests still pass
SeanKilleen 507d2cb
Invert the if statements for compactness / clarity
SeanKilleen a9b9437
Test subclasses & reformatting
SeanKilleen fa7ffc7
Test name changes
SeanKilleen 4076e08
Whitespace fix
SeanKilleen 6927842
Remove redundant line
SeanKilleen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
using System.Collections.Generic; | ||
using System.Net; | ||
using Octokit.Internal; | ||
using Xunit; | ||
|
||
namespace Octokit.Tests.Exceptions | ||
{ | ||
public class AbuseExceptionTests | ||
{ | ||
public class TheConstructor | ||
{ | ||
public class Message | ||
{ | ||
[Fact] | ||
public void ContainsAbuseMessageFromApi() | ||
{ | ||
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 RetryAfterSeconds | ||
{ | ||
public class HappyPath | ||
{ | ||
[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 WithZeroHeaderValue_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); | ||
} | ||
} | ||
|
||
public class UnhappyPaths | ||
{ | ||
[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); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
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 | ||
{ | ||
/// <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"); | ||
|
||
RetryAfterSeconds = ParseRetryAfterSeconds(response); | ||
} | ||
|
||
private static int? ParseRetryAfterSeconds(IResponse response) | ||
{ | ||
string secondsValue; | ||
if (!response.Headers.TryGetValue("Retry-After", out secondsValue)) { return null; } | ||
|
||
int retrySeconds; | ||
if (!int.TryParse(secondsValue, out retrySeconds)) { return null; } | ||
if (retrySeconds < 0) { return null; } | ||
|
||
return retrySeconds; | ||
} | ||
|
||
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); | ||
} | ||
#endif | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 👍