Skip to content
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

Add Server Time Difference to ApiInfo #2196

Merged
merged 5 commits into from
Jun 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions Octokit.Tests/Exceptions/RateLimitExceededExceptionTests.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
using System;
using Octokit.Internal;

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Net;
using System.Net.Http;
#if !NO_SERIALIZABLE
using System.IO;
using System.Runtime.Serialization.Formatters.Binary;
Expand Down Expand Up @@ -47,7 +50,9 @@ public void HandlesInvalidHeaderValues()
{
{"X-RateLimit-Limit", "XXX"},
{"X-RateLimit-Remaining", "XXXX"},
{"X-RateLimit-Reset", "XXXX"}
{"X-RateLimit-Reset", "XXXX"},
{"Date", "XXX"},
{ApiInfoParser.ReceivedTimeHeaderName, "XXX"},
};
var response = CreateResponse(HttpStatusCode.Forbidden, headers);

Expand All @@ -61,6 +66,7 @@ public void HandlesInvalidHeaderValues()
"ddd dd MMM yyyy h:mm:ss tt zzz",
CultureInfo.InvariantCulture);
Assert.Equal(expectedReset, exception.Reset);
Assert.Equal(TimeSpan.Zero, exception.GetRetryAfterTimeSpan());
}

[Fact]
Expand All @@ -77,6 +83,7 @@ public void HandlesMissingHeaderValues()
"ddd dd MMM yyyy h:mm:ss tt zzz",
CultureInfo.InvariantCulture);
Assert.Equal(expectedReset, exception.Reset);
Assert.Equal(TimeSpan.Zero, exception.GetRetryAfterTimeSpan());
}

#if !NO_SERIALIZABLE
Expand Down Expand Up @@ -111,5 +118,58 @@ public void CanPopulateObjectFromSerializedData()
}
#endif
}

public class GetRetryAfterTimeSpanMethod
{
[Fact]
public void ReturnsSkewedDistanceFromReset()
{
// One hour into the future is hopefully long enough to still be
// in the future at the end of this method
var resetTime = DateTimeOffset.Now + TimeSpan.FromHours(1);
var recvDate = new DateTimeOffset(2020, 06, 07, 12, 00, 00, TimeSpan.Zero);
var skew = TimeSpan.FromSeconds(-5);

var response = CreateResponse(HttpStatusCode.Forbidden,
new Dictionary<string, string>
{
["X-RateLimit-Limit"] = "100",
["X-RateLimit-Remaining"] = "0",
["X-RateLimit-Reset"] = resetTime.ToUnixTimeSeconds()
.ToString(CultureInfo.InvariantCulture),
["Date"] = (recvDate + skew).ToString("r"),
[ApiInfoParser.ReceivedTimeHeaderName] = recvDate.ToString("r"),
});
Assert.Equal(skew, response.ApiInfo.ServerTimeDifference);
var except = new RateLimitExceededException(response);

var timeToReset = except.GetRetryAfterTimeSpan();
Assert.NotEqual(TimeSpan.Zero, timeToReset);
Assert.InRange(timeToReset, TimeSpan.Zero, TimeSpan.FromHours(1));
}

[Fact]
public void ReturnsZeroIfSkewedResetInPast()
{
var beginTime = DateTimeOffset.Now;
var resetTime = beginTime - TimeSpan.FromHours(1);

var response = CreateResponse(HttpStatusCode.Forbidden,
new Dictionary<string, string>
{
["X-RateLimit-Limit"] = "100",
["X-RateLimit-Remaining"] = "0",
["X-RateLimit-Reset"] = resetTime.ToUnixTimeSeconds()
.ToString(CultureInfo.InvariantCulture),
["Date"] = beginTime.ToString("r"),
[ApiInfoParser.ReceivedTimeHeaderName] = beginTime.ToString("r"),
});
Assert.Equal(TimeSpan.Zero, response.ApiInfo.ServerTimeDifference);
var except = new RateLimitExceededException(response);

var timeToReset = except.GetRetryAfterTimeSpan();
Assert.Equal(TimeSpan.Zero, timeToReset);
}
}
}
}
66 changes: 66 additions & 0 deletions Octokit.Tests/Http/ApiInfoParserTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,72 @@ public void ParsesLinkHeader()
Assert.Equal(new Uri("https://api.github.com/repos/rails/rails/issues?page=131&per_page=5"),
apiInfo.Links["last"]);
}

[Fact]
public void ParsesServerTimeDifference()
{
var serverDate = new DateTimeOffset(2020, 06, 07, 12, 00, 00, TimeSpan.Zero);
var receivedDate = new DateTimeOffset(2020, 06, 07, 14, 00, 00, TimeSpan.Zero);
var diff = serverDate - receivedDate;

var headers = new Dictionary<string, string>
{
["Date"] = serverDate.ToString("r"), // Format string r: RFC1123 HTTP Round-tripping time format
[ApiInfoParser.ReceivedTimeHeaderName] = receivedDate.ToString("r")
};

var apiInfo = ApiInfoParser.ParseResponseHeaders(headers);

Assert.NotNull(apiInfo);
Assert.Equal(diff, apiInfo.ServerTimeDifference);
}

[Fact]
public void ParsesServerTimeDifferenceAsZeroWhenDateHeaderIsMissing()
{
var receivedDate = new DateTimeOffset(2020, 06, 07, 14, 00, 00, TimeSpan.Zero);
var headers = new Dictionary<string, string>
{
// Format string r: RFC1123 HTTP Round-tripping time format
[ApiInfoParser.ReceivedTimeHeaderName] = receivedDate.ToString("r")
};

var apiInfo = ApiInfoParser.ParseResponseHeaders(headers);

Assert.NotNull(apiInfo);
Assert.Equal(TimeSpan.Zero, apiInfo.ServerTimeDifference);
}

[Fact]
public void ParsesServerTimeDifferenceAsZeroWhenReceiveDateHeaderIsNonesense()
{
var headers = new Dictionary<string, string>
{
// Format string r: RFC1123 HTTP Round-tripping time format
["Date"] = DateTimeOffset.Now.ToString("r"),
[ApiInfoParser.ReceivedTimeHeaderName] = "abfhjsdkhfjkldhf"
};

var apiInfo = ApiInfoParser.ParseResponseHeaders(headers);

Assert.NotNull(apiInfo);
Assert.Equal(TimeSpan.Zero, apiInfo.ServerTimeDifference);
}

[Fact]
public void ParsesServerTimeDifferenceAsZeroWhenReceiveDateHeaderIsMissing()
{
var serverDate = new DateTimeOffset(2020, 06, 07, 12, 00, 00, TimeSpan.Zero);
var headers = new Dictionary<string, string>
{
["Date"] = serverDate.ToString("r"), // Format string r: RFC1123 HTTP Round-tripping time format
};

var apiInfo = ApiInfoParser.ParseResponseHeaders(headers);

Assert.NotNull(apiInfo);
Assert.Equal(TimeSpan.Zero, apiInfo.ServerTimeDifference);
}
}

public class ThePageUrlMethods
Expand Down
27 changes: 27 additions & 0 deletions Octokit/Exceptions/RateLimitExceededException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ namespace Octokit
public class RateLimitExceededException : ForbiddenException
{
readonly RateLimit _rateLimit;
readonly TimeSpan _severTimeDiff = TimeSpan.Zero;

/// <summary>
/// Constructs an instance of RateLimitExceededException
Expand All @@ -45,6 +46,8 @@ public RateLimitExceededException(IResponse response, Exception innerException)
Ensure.ArgumentNotNull(response, nameof(response));

_rateLimit = response.ApiInfo.RateLimit;

_severTimeDiff = response.ApiInfo.ServerTimeDifference;
}

/// <summary>
Expand Down Expand Up @@ -78,6 +81,27 @@ public override string Message
get { return ApiErrorMessageSafe ?? "API Rate Limit exceeded"; }
}

/// <summary>
/// Calculates the time from now to wait until the next request can be
/// attempted.
/// </summary>
/// <returns>
/// A non-negative <see cref="TimeSpan"/> value. Returns
/// <see cref="TimeSpan.Zero"/> if the next Rate Limit window has
/// started and the next request can be attempted immediately.
/// </returns>
/// <remarks>
/// The return value is calculated using server time data from the
/// response in order to provide a best-effort estimate that is
/// independant from eventual inaccuracies in the client's clock.
/// </remarks>
public TimeSpan GetRetryAfterTimeSpan()
Copy link
Member

@shiftkey shiftkey Jun 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a check to the existing HandlesMissingHeaderValues test inside Octokit.Tests/Exceptions/RateLimitExceededExceptionTests.cs, as well as a new test using the expected headers to ensure we're not erroring somehow when doing the parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shiftkey I added new Assertions to the existing test and added new unit tests for both the Exception and the ApiInfoParser class.

{
var skewedResetTime = Reset + _severTimeDiff;
var ts = skewedResetTime - DateTimeOffset.Now;
return ts > TimeSpan.Zero ? ts : TimeSpan.Zero;
}

#if !NO_SERIALIZABLE
/// <summary>
/// Constructs an instance of RateLimitExceededException
Expand All @@ -95,6 +119,8 @@ protected RateLimitExceededException(SerializationInfo info, StreamingContext co
{
_rateLimit = info.GetValue("RateLimit", typeof(RateLimit)) as RateLimit
?? new RateLimit(new Dictionary<string, string>());
if (info.GetValue(nameof(ApiInfo.ServerTimeDifference), typeof(TimeSpan)) is TimeSpan serverTimeDiff)
_severTimeDiff = serverTimeDiff;
}

[SecurityCritical]
Expand All @@ -103,6 +129,7 @@ public override void GetObjectData(SerializationInfo info, StreamingContext cont
base.GetObjectData(info, context);

info.AddValue("RateLimit", _rateLimit);
info.AddValue(nameof(ApiInfo.ServerTimeDifference), _severTimeDiff);
}
#endif
}
Expand Down
18 changes: 16 additions & 2 deletions Octokit/Http/ApiInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ public ApiInfo(IDictionary<string, Uri> links,
IList<string> oauthScopes,
IList<string> acceptedOauthScopes,
string etag,
RateLimit rateLimit)
RateLimit rateLimit,
TimeSpan serverTimeDifference = default)
{
Ensure.ArgumentNotNull(links, nameof(links));
Ensure.ArgumentNotNull(oauthScopes, nameof(oauthScopes));
Expand All @@ -24,6 +25,7 @@ public ApiInfo(IDictionary<string, Uri> links,
AcceptedOauthScopes = new ReadOnlyCollection<string>(acceptedOauthScopes);
Etag = etag;
RateLimit = rateLimit;
ServerTimeDifference = serverTimeDifference;
}

/// <summary>
Expand Down Expand Up @@ -51,6 +53,17 @@ public ApiInfo(IDictionary<string, Uri> links,
/// </summary>
public RateLimit RateLimit { get; private set; }

/// <summary>
/// The best-effort time difference between the server and the client.
/// </summary>
/// <remarks>
/// If both the server and the client have reasonably accurate clocks,
/// the value of this property will be close to <see cref="TimeSpan.Zero"/>.
/// The actual value is sensitive to network transmission and processing
/// delays.
/// </remarks>
public TimeSpan ServerTimeDifference { get; }

/// <summary>
/// Allows you to clone ApiInfo
/// </summary>
Expand All @@ -61,7 +74,8 @@ public ApiInfo Clone()
OauthScopes.Clone(),
AcceptedOauthScopes.Clone(),
Etag != null ? new string(Etag.ToCharArray()) : null,
RateLimit != null ? RateLimit.Clone() : null);
RateLimit != null ? RateLimit.Clone() : null,
ServerTimeDifference);
}
}
}
14 changes: 13 additions & 1 deletion Octokit/Http/ApiInfoParser.cs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text.RegularExpressions;

namespace Octokit.Internal
{
internal static class ApiInfoParser
{
public const string ReceivedTimeHeaderName = "X-Octokit-ReceivedDate";

const RegexOptions regexOptions =
#if HAS_REGEX_COMPILED_OPTIONS
RegexOptions.Compiled |
Expand Down Expand Up @@ -73,7 +76,16 @@ public static ApiInfo ParseResponseHeaders(IDictionary<string, string> responseH
}
}

return new ApiInfo(httpLinks, oauthScopes, acceptedOauthScopes, etag, new RateLimit(responseHeaders));
var receivedTimeKey = LookupHeader(responseHeaders, ReceivedTimeHeaderName);
var serverTimeKey = LookupHeader(responseHeaders, "Date");
TimeSpan serverTimeSkew = TimeSpan.Zero;
if (DateTimeOffset.TryParse(receivedTimeKey.Value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var receivedTime)
&& DateTimeOffset.TryParse(serverTimeKey.Value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var serverTime))
{
serverTimeSkew = serverTime - receivedTime;
}

return new ApiInfo(httpLinks, oauthScopes, acceptedOauthScopes, etag, new RateLimit(responseHeaders), serverTimeSkew);
}
}
}
24 changes: 22 additions & 2 deletions Octokit/Http/HttpClientAdapter.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Net;
Expand Down Expand Up @@ -123,10 +125,22 @@ protected virtual async Task<IResponse> BuildResponse(HttpResponseMessage respon
}
}

var responseHeaders = responseMessage.Headers.ToDictionary(h => h.Key, h => h.Value.First());

// Add Client response received time as a synthetic header
const string receivedTimeHeaderName = ApiInfoParser.ReceivedTimeHeaderName;
if (responseMessage.RequestMessage?.Properties is IDictionary<string, object> reqProperties
&& reqProperties.TryGetValue(receivedTimeHeaderName, out object receivedTimeObj)
&& receivedTimeObj is string receivedTimeString
&& !responseHeaders.ContainsKey(receivedTimeHeaderName))
{
responseHeaders[receivedTimeHeaderName] = receivedTimeString;
}

return new Response(
responseMessage.StatusCode,
responseBody,
responseMessage.Headers.ToDictionary(h => h.Key, h => h.Value.First()),
responseHeaders,
contentType);
}

Expand Down Expand Up @@ -203,7 +217,13 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, Can
var clonedRequest = await CloneHttpRequestMessageAsync(request).ConfigureAwait(false);

// Send initial response
var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false);
var response = await _http.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
// Need to determine time on client computer as soon as possible.
var receivedTime = DateTimeOffset.Now;
// Since Properties are stored as objects, serialize to HTTP round-tripping string (Format: r)
// Resolution is limited to one-second, matching the resolution of the HTTP Date header
request.Properties[ApiInfoParser.ReceivedTimeHeaderName] =
receivedTime.ToString("r", CultureInfo.InvariantCulture);

// Can't redirect without somewhere to redirect to.
if (response.Headers.Location == null)
Expand Down