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

[WIP] implement first-class paging support #740

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 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
1 change: 1 addition & 0 deletions CustomDictionary.xml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<Word>Mergeable</Word>
<Word>Symlink</Word>
<Word>Submodule</Word>
<Word>Awaiter</Word>
</Recognized>
</Words>
<Acronyms>
Expand Down
1 change: 1 addition & 0 deletions Octokit.Tests.Conventions/SyncObservableClients.cs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ private static Type GetObservableExpectedType(Type mainType)
case TypeCategory.GenericTask:
// single item - Task<TResult> => IObservable<TResult>
case TypeCategory.ReadOnlyList:
case TypeCategory.LazyReadOnlyList:
// list - Task<IReadOnlyList<TResult>> => IObservable<TResult>
return typeof(IObservable<>).MakeGenericType(typeInfo.Type);
case TypeCategory.Other:
Expand Down
14 changes: 13 additions & 1 deletion Octokit.Tests.Conventions/TypeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ public static TypeInfo GetTypeInfo(this Type type)
{
typeInfo.TypeCategory = TypeCategory.ClientInterface;
}
else if (type.IsLazyRequest())
{
typeInfo.TypeCategory = TypeCategory.LazyReadOnlyList;
typeInfo.Type = type.GetGenericArgument();
}
else if(type.IsTask())
{
if(!type.IsGenericType)
Expand Down Expand Up @@ -85,6 +90,13 @@ public static Type GetObservableClientInterface(this Type type)
return observableInterface;
}

public static bool IsLazyRequest(this Type type)
{
if (!type.IsGenericType) return false;
var genericType = type.GetGenericTypeDefinition();
return typeof(IDeferredRequest<>).IsAssignableFrom(genericType);
}

public static bool IsTask(this Type type)
{
return typeof(Task).IsAssignableFrom(type);
Expand All @@ -101,7 +113,7 @@ public static Type GetGenericArgument(this Type type)
}
}

public enum TypeCategory { Other, Task, GenericTask, ReadOnlyList, ClientInterface }
public enum TypeCategory { Other, Task, GenericTask, ReadOnlyList, LazyReadOnlyList, ClientInterface }

public struct TypeInfo
{
Expand Down
11 changes: 11 additions & 0 deletions Octokit.Tests.Integration/Clients/RepositoriesClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,17 @@ public async Task ReturnsForkedRepository()
Assert.Equal("https://github.com/Haacked/libgit2sharp.git", repository.CloneUrl);
Assert.True(repository.Fork);
}

[IntegrationTest]
public async Task CanPageRepositories()
{
var github = Helper.GetAuthenticatedClient();

var repositories = await github.Repository.GetAllForCurrent()
.WithOptions(new ApiOptions { PageSize = 10, PageCount = 1 });

Assert.Equal(10, repositories.Count);
}
}

public class TheGetAllForOrgMethod
Expand Down
64 changes: 50 additions & 14 deletions Octokit.Tests/Clients/RepositoriesClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -263,61 +263,97 @@ public async Task EnsuresNonNullArguments()
public class TheGetAllForCurrentMethod
{
[Fact]
public void RequestsTheCorrectUrlAndReturnsRepositories()
public async Task RequestsTheCorrectUrlAndReturnsRepositories()
{
var response = Substitute.For<IResponse>();
response.ApiInfo.Returns(ApiInfo.Empty);

var emptySet = Substitute.For<IApiResponse<List<Repository>>>();
emptySet.Body.Returns(new List<Repository>());
emptySet.HttpResponse.Returns(response);

var connection = Substitute.For<IApiConnection>();
connection.Connection.Get<List<Repository>>(Args.Uri, Args.EmptyDictionary, null)
.Returns(Task.FromResult(emptySet));
var client = new RepositoriesClient(connection);

client.GetAllForCurrent();
await client.GetAllForCurrent();

connection.Received()
.GetAll<Repository>(Arg.Is<Uri>(u => u.ToString() == "user/repos"));
connection.Connection.Received()
.Get<List<Repository>>(
Arg.Is<Uri>(u => u.ToString() == "user/repos"),
Args.EmptyDictionary,
null);
}
}

public class TheGetAllForUserMethod
{
[Fact]
public void RequestsTheCorrectUrlAndReturnsRepositories()
public async Task RequestsTheCorrectUrlAndReturnsRepositories()
{
var response = Substitute.For<IResponse>();
response.ApiInfo.Returns(ApiInfo.Empty);

var emptySet = Substitute.For<IApiResponse<List<Repository>>>();
emptySet.Body.Returns(new List<Repository>());
emptySet.HttpResponse.Returns(response);

var connection = Substitute.For<IApiConnection>();
connection.Connection.Get<List<Repository>>(Args.Uri, Args.EmptyDictionary, null)
.Returns(Task.FromResult(emptySet));
var client = new RepositoriesClient(connection);

client.GetAllForUser("username");
await client.GetAllForUser("username");

connection.Received()
.GetAll<Repository>(Arg.Is<Uri>(u => u.ToString() == "users/username/repos"));
connection.Connection.Received()
.Get<List<Repository>>(
Arg.Is<Uri>(u => u.ToString() == "users/username/repos"),
Args.EmptyDictionary,
null);
}

[Fact]
public async Task EnsuresNonNullArguments()
{
var reposEndpoint = new RepositoriesClient(Substitute.For<IApiConnection>());

await Assert.ThrowsAsync<ArgumentNullException>(() => reposEndpoint.GetAllForUser(null));
await Assert.ThrowsAsync<ArgumentNullException>(() => reposEndpoint.GetAllForUser(null).ToTask());
}
}

public class TheGetAllForOrgMethod
{
[Fact]
public void RequestsTheCorrectUrlAndReturnsRepositories()
public async Task RequestsTheCorrectUrlAndReturnsRepositories()
{
var response = Substitute.For<IResponse>();
response.ApiInfo.Returns(ApiInfo.Empty);

var emptySet = Substitute.For<IApiResponse<List<Repository>>>();
emptySet.Body.Returns(new List<Repository>());
emptySet.HttpResponse.Returns(response);

var connection = Substitute.For<IApiConnection>();
connection.Connection.Get<List<Repository>>(Args.Uri, Args.EmptyDictionary, null)
.Returns(Task.FromResult(emptySet));
var client = new RepositoriesClient(connection);

client.GetAllForOrg("orgname");
await client.GetAllForOrg("orgname");

connection.Received()
.GetAll<Repository>(Arg.Is<Uri>(u => u.ToString() == "orgs/orgname/repos"));
connection.Connection.Received()
.Get<List<Repository>>(
Arg.Is<Uri>(u => u.ToString() == "orgs/orgname/repos"),
Args.EmptyDictionary,
null);
}

[Fact]
public async Task EnsuresNonNullArguments()
{
var reposEndpoint = new RepositoriesClient(Substitute.For<IApiConnection>());

await Assert.ThrowsAsync<ArgumentNullException>(() => reposEndpoint.GetAllForOrg(null));
await Assert.ThrowsAsync<ArgumentNullException>(() => reposEndpoint.GetAllForOrg(null).ToTask());
}
}

Expand Down
7 changes: 4 additions & 3 deletions Octokit/Clients/IRepositoriesClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ public interface IRepositoriesClient
/// <returns>A <see cref="IReadOnlyPagedCollection{Repository}"/> of <see cref="Repository"/>.</returns>
[SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate",
Justification = "Makes a network request")]
Task<IReadOnlyList<Repository>> GetAllForCurrent();
IDeferredRequest<Repository> GetAllForCurrent();


/// <summary>
/// Gets all repositories owned by the specified user.
Expand All @@ -120,7 +121,7 @@ public interface IRepositoriesClient
/// <returns>A <see cref="IReadOnlyPagedCollection{Repository}"/> of <see cref="Repository"/>.</returns>
[SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate",
Justification = "Makes a network request")]
Task<IReadOnlyList<Repository>> GetAllForUser(string login);
IDeferredRequest<Repository> GetAllForUser(string login);

/// <summary>
/// Gets all repositories owned by the specified organization.
Expand All @@ -133,7 +134,7 @@ public interface IRepositoriesClient
/// <returns>A <see cref="IReadOnlyPagedCollection{Repository}"/> of <see cref="Repository"/>.</returns>
[SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate",
Justification = "Makes a network request")]
Task<IReadOnlyList<Repository>> GetAllForOrg(string organization);
IDeferredRequest<Repository> GetAllForOrg(string organization);

/// <summary>
/// Gets the preferred README for the specified repository.
Expand Down
12 changes: 6 additions & 6 deletions Octokit/Clients/RepositoriesClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,9 @@ public Task<Repository> Get(string owner, string name)
/// <exception cref="AuthorizationException">Thrown if the client is not authenticated.</exception>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>A <see cref="IReadOnlyPagedCollection{Repository}"/> of <see cref="Repository"/>.</returns>
public Task<IReadOnlyList<Repository>> GetAllForCurrent()
public IDeferredRequest<Repository> GetAllForCurrent()
{
return ApiConnection.GetAll<Repository>(ApiUrls.Repositories());
return new DeferredRequest<Repository>(ApiConnection, ApiUrls.Repositories());
}

/// <summary>
Expand All @@ -198,11 +198,11 @@ public Task<IReadOnlyList<Repository>> GetAllForCurrent()
/// </remarks>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>A <see cref="IReadOnlyPagedCollection{Repository}"/> of <see cref="Repository"/>.</returns>
public Task<IReadOnlyList<Repository>> GetAllForUser(string login)
public IDeferredRequest<Repository> GetAllForUser(string login)
{
Ensure.ArgumentNotNullOrEmptyString(login, "login");

return ApiConnection.GetAll<Repository>(ApiUrls.Repositories(login));
return new DeferredRequest<Repository>(ApiConnection, ApiUrls.Repositories(login));
}

/// <summary>
Expand All @@ -214,11 +214,11 @@ public Task<IReadOnlyList<Repository>> GetAllForUser(string login)
/// </remarks>
/// <exception cref="ApiException">Thrown when a general API error occurs.</exception>
/// <returns>A <see cref="IReadOnlyPagedCollection{Repository}"/> of <see cref="Repository"/>.</returns>
public Task<IReadOnlyList<Repository>> GetAllForOrg(string organization)
public IDeferredRequest<Repository> GetAllForOrg(string organization)
{
Ensure.ArgumentNotNullOrEmptyString(organization, "organization");

return ApiConnection.GetAll<Repository>(ApiUrls.OrganizationRepositories(organization));
return new DeferredRequest<Repository>(ApiConnection, ApiUrls.OrganizationRepositories(organization));
}

/// <summary>
Expand Down
8 changes: 8 additions & 0 deletions Octokit/Http/ApiInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ namespace Octokit
/// </summary>
public class ApiInfo
{
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Security", "CA2104:DoNotDeclareReadOnlyMutableReferenceTypes")]
public static readonly ApiInfo Empty = new ApiInfo(
new Dictionary<string, Uri>(),
new string[0],
new string[0],
"",
new RateLimit(new Dictionary<string, string>()));

public ApiInfo(IDictionary<string, Uri> links,
IList<string> oauthScopes,
IList<string> acceptedOauthScopes,
Expand Down
15 changes: 15 additions & 0 deletions Octokit/Http/CustomAwaiter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace Octokit
{
public static class CustomAwaiter
{
public static TaskAwaiter<IReadOnlyList<T>> GetAwaiter<T>(this IDeferredRequest<T> request)
Copy link
Member Author

Choose a reason for hiding this comment

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

So I could 🔥 this awaiter and make the user call .ToTask() for these methods.

It's less magical, but takes away the hot/cold issue.

Choose a reason for hiding this comment

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

Still a breaking change. The API itself has changed from hot to cold and existing code now needs to .ToTask()

Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure I can magic up a type here that's both Hot and also modifiable using WithOptions.

So I'm weighing up varying degrees of breaking changes to see how they feel.

Copy link
Member Author

Choose a reason for hiding this comment

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

The API itself has changed from hot to cold and existing code now needs to .ToTask()

Yes, any existing API calls will no longer magically work with await. So you're updating anyway.

{
Ensure.ArgumentNotNull(request, "request");

return request.ToTask().GetAwaiter();
}
}
}
112 changes: 112 additions & 0 deletions Octokit/Http/DeferredRequest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Octokit.Internal;
using System.Globalization;
using System.Diagnostics.CodeAnalysis;

namespace Octokit
{
public class DeferredRequest<T> : IDeferredRequest<T>
{
readonly IApiPagination _pagination = new ApiPagination();
readonly IApiConnection _apiConnection;

readonly Uri _uri;
readonly IDictionary<string, string> _parameters;

ApiOptions _options = new ApiOptions();

public DeferredRequest(IApiConnection apiConnection, Uri uri)
: this(apiConnection, uri, new Dictionary<string, string>()) { }

public DeferredRequest(IApiConnection apiConnection, Uri uri, IDictionary<string, string> parameters)
{
_apiConnection = apiConnection;
_uri = uri;
_parameters = parameters;
}

public IDeferredRequest<T> WithOptions(ApiOptions options)
{
this._options = options;
return this;
}

public Task<IReadOnlyList<T>> ToTask()
{
if (_options.PageSize.HasValue)
{
_parameters.Add("per_page", _options.PageSize.Value.ToString(CultureInfo.InvariantCulture));
}

if (_options.StartPage.HasValue)
{
_parameters.Add("page", _options.StartPage.Value.ToString(CultureInfo.InvariantCulture));
}

return _pagination.GetAllPages(
async () => await GetPage<T>(_uri, _parameters, null).ConfigureAwait(false), _uri);
}


async Task<IReadOnlyPagedCollection<TU>> GetPage<TU>(
Uri uri,
IDictionary<string, string> parameters,
string accepts)
{
Ensure.ArgumentNotNull(uri, "uri");

var connection = _apiConnection.Connection;

var response = await connection.Get<List<TU>>(uri, parameters, accepts).ConfigureAwait(false);
return new ReadOnlyPagedCollection<TU>(
response,
nextPageUri =>
{
if (nextPageUri.Query.Contains("page=") && _options.PageCount.HasValue)
{
var allValues = ToQueryStringDictionary(nextPageUri);

string pageValue;
if (allValues.TryGetValue("page", out pageValue))
{
var startPage = _options.StartPage ?? 1;
var pageCount = _options.PageCount.Value;

var endPage = startPage + pageCount;
if (pageValue.Equals(endPage.ToString(), StringComparison.OrdinalIgnoreCase))
{
return null;
}
}
}

return connection.Get<List<TU>>(nextPageUri, parameters, accepts);
});
}

static Dictionary<string, string> ToQueryStringDictionary(Uri uri)
{
var allValues = uri.Query.Split('&')
.Select(keyValue =>
{
var indexOf = keyValue.IndexOf('=');
if (indexOf > 0)
{
var key = keyValue.Substring(0, indexOf);
var value = keyValue.Substring(indexOf + 1);
return new KeyValuePair<string, string>(key, value);
}

//just a plain old value, return it
return new KeyValuePair<string, string>(keyValue, null);
})
.ToDictionary(x => x.Key, x => x.Value);
return allValues;
}
}
}
Loading